Editing Synthetix (section)

== Bad engineering practices ==

A cursory glance of the Synthetix solidity code shows poor coding practices. For instance: there are multiple places within the code that try to hide errors and anticipate what the user wants rather than crashing (with potentially disastrous consequences.)

One such example of this is the code used to check balances for exchanges. You would expect that if a user doesn’t have enough funds to cover the conversion that the software would crash with some kind of error. But Synthetix defaults to using all of their remaining balance. This is bad because the user might have made a mistake like accidentally getting the order of the pairs backwards or putting a decimal in the wrong place.

[hide_error]

<pre class="solidity">        // when there isn't enough supply (either due to reclamation settlement or because the number is too high)
        if (amountAfterSettlement &gt; balanceOfSourceAfterSettlement) {
            // then the amount to exchange is reduced to their remaining supply
            amountAfterSettlement = balanceOfSourceAfterSettlement;
        }</pre>
Another example is in the issuance code which seems to have some kind of weird logic to detect edge cases where users were ‘accidentally’ allocated more than they should have from an exchange (WTF?) Much of the code is a mind-fuck moment in general.

You can see that the price quotes are divided up into discrete time periods. It wasn’t my job to do a code audit for this project – I only glanced where there was ambiguity – but I have a feeling that the settlement logic around these price periods / edge-cases may be directly vulnerable.

[edge_case]

<pre class="solidity">// and deduct the fee from this amount using the exchangeFeeRate from storage
uint amountShouldHaveReceived = _getAmountReceivedForExchange(destinationAmount, exchangeEntry.exchangeFeeRate);

// SIP-65 settlements where the amount at end of waiting period is beyond the threshold, then
// settle with no reclaim or rebate
if (!_isDeviationAboveThreshold(exchangeEntry.amountReceived, amountShouldHaveReceived)) {
	if (exchangeEntry.amountReceived &gt; amountShouldHaveReceived) {
		// if they received more than they should have, add to the reclaim tally
		reclaim = exchangeEntry.amountReceived.sub(amountShouldHaveReceived);
		reclaimAmount = reclaimAmount.add(reclaim);
	} else if (amountShouldHaveReceived &gt; exchangeEntry.amountReceived) {
		// if less, add to the rebate tally
		rebate = amountShouldHaveReceived.sub(exchangeEntry.amountReceived);
		rebateAmount = rebateAmount.add(rebate);
	}
}</pre>
<span id="misleading-claims"></span>