Unnecessary conditions causing Over Gas consumption

[code423n4]

Handle

csanuragjain

Vulnerability details

Impact

Gas wastage, can be optimized with changing condition

Proof of Concept

1. Navigate to https://github.com/code-423n4/2021-10-tally/blob/main/contracts/swap/Swap.sol

2. Let us see the swapByQuote function which has below condition

require(
            (
                !signifiesETHOrZero(zrxBuyTokenAddress) &&
                boughtERC20Amount >= minimumAmountReceived
            ) ||
            (
                signifiesETHOrZero(zrxBuyTokenAddress) &&
                boughtETHAmount >= minimumAmountReceived
            ),
            "Swap::swapByQuote: Minimum swap proceeds requirement not met"
        );

3. Now there are only 2 types of token which are ETH and non ETH token.

4. But the condition in 2 is splitting the checks based on type of token which can be optimized. Since we already know that tokens can be ETH or non ETH so simply we can change the condition to

require(
            (
                boughtERC20Amount >= minimumAmountReceived
            ) ||
            (
                boughtETHAmount >= minimumAmountReceived
            ),
            "Swap::swapByQuote: Minimum swap proceeds requirement not met"
        );

Tools Used

Recommended Mitigation Steps

Change the condition to below:

require(
            (
                boughtERC20Amount >= minimumAmountReceived
            ) ||
            (
                boughtETHAmount >= minimumAmountReceived
            ),
            "Swap::swapByQuote: Minimum swap proceeds requirement not met"
        );

[Shadowfiend]

This feels correct, though the additional verification of the pair (token swap + token amount > minimum, ETH swap + ETH amount > minimum) feels like a good way to avoid having to reason about potentially complex interactions between ETH and token amounts in edge cases. We may leave this as-is.