Handle

harleythedog

Vulnerability details

Impact

Suppose that swapByQuote is called with zrxSellTokenAddress == zrxBuyTokenAddress, and neither of these addresses "signifiesETHOrZero". The contract first transfers amountToSell of these tokens from the sender's account into the contract and updates the allowance. Then, fillZrxQuote is called, and the erc20Delta variable is calculated as

(amount of the buy token after the zrxTo.call) - (amount of the buy token before the zrxTo.call)

So, if the zrxTo.call resulted in 0 output (trades amountToSell buy tokens for amountToSell buy tokens), then erc20Delta will be calculated as 0. This will throw an error due to the require statement on line 219, so the swap will not proceed.

There are 2 trains of thought here:

1) It shouldn't be the case that zrxSellTokenAddress == zrxBuyTokenAddress anyways, so it is okay that this errors. If this is the case, I believe adding a require(zrxSellTokenAddress != zrxBuyTokenAddress) at the start of the swapByQuote function would be beneficial, as there is currently no explicit checks for this, and this would save the gas of having to go through a lot of the code before erroring out.

2) This is a valid issue, and users should be able to, for example, trade xyz tokens for xyz tokens using swapByQuote. In this case, the calculation of erc20Delta should instead be

(amount of buy token after zrxTo.call) - (amount of buy token before transferring the sell tokens)

Side note: In case 2, the scenario where a user wants to trade unwrapped eth for unwrapped eth also needs to be handled differently, since address(this).balance is incremented with msg.value before swapByQuote even begins executing.

Proof of Concept

fillZrxQuote here: https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol#:~:text=function-,fillZrxQuote,-(

Recommend setting originalERC20Balance at the start of swapByQuote instead, and then calculate erc20Delta in swapByQuote as well using the logic described above.

Or, if this behaviour is not intended, explicitly check that zrxSellTokenAddress != zrxBuyTokenAddress in swapByQuote to make this more obvious and potentially save gas on these cases.

Tools Used

Manual inspection.

Recommended Mitigation Steps

Handle case where sell token and buy token are equal as described above (or make this more explicitly disallowed if this is not expected behaviour).

code-423n4 / 2021-10-tally-findings