Tally contest - cmichel's results

Handle

cmichel

Vulnerability details

The Swap.constructor allows arbitrary swap fees, even > 100%. It should check that the fee is at most 100% as in setSwapFee.

Impact

Setting swap fees > 100% will revert the contract on trades when trying to pay out more than received.

Recommended Mitigation Steps

Consider adding a require(swapFee_ < SWAP_FEE_DIVISOR, "Swap::setSwapFee: Swap fee must not exceed 100%"); statement to the constructor.

Handle

cmichel

Vulnerability details

There exist ERC20 tokens that made certain customizations to their contract. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom().

The Swap.swapByQuote function performs a safeTransferFrom(msg.sender, this, amountToSell) and then approves the same amountToSell which will be more than what the Swap contract actually received due to fees.

Impact

The zrxAllowanceTarget contract is allowed to transfer more than what was transferred to the contract post fees. If the Swap contract accumulated swap fees in this token, part of them (amountToSell - amountToSellPostFees) can be stolen/are allowed to be transferred from the Swap contract.

Recommended Mitigation Steps

One possible mitigation is to measure the asset change right before and after the asset-transferring IERC20(zrxSellTokenAddress).safeTransferFrom(msg.sender, address(this), amountToSell) call, and then only approve the zrxAllowanceTarget with the difference.

Handle

cmichel

Vulnerability details

The Math library uses Solidity version 0.8 which comes with built-in overflow checks which cost gas.

The code already checks for underflows (a > b before doing the division), and therefore the built-in checks can be disabled everywhere for improved gas cost.

pragma solidity ^0.8.0;

library Math {
    function subOrZero(uint256 a, uint256 b) internal pure returns (uint256) {
      unchecked {
        return a > b ? a - b : 0;
      }
    }

    function subOrZero(uint128 a, uint128 b) internal pure returns (uint128) {
      unchecked {
        return a > b ? a - b : 0;
      }
    }

    function subOrZero(uint64 a, uint64 b) internal pure returns (uint64) {
      unchecked {
        return a > b ? a - b : 0;
      }
    }

    function subOrZero(uint32 a, uint32 b) internal pure returns (uint32) {
      unchecked {
        return a > b ? a - b : 0;
      }
    }
}

Handle

cmichel

Vulnerability details

The Swap contract uses Solidity version 0.8 which already implements overflow checks by default. At the same time, it uses the SafeMath library which is more gas expensive than the 0.8 overflow checks.

It should just use the built-in checks and remove SafeMath from the dependencies:

// @audit can just normal arithmetic here
uint256 toTransfer = SWAP_FEE_DIVISOR.sub(swapFee).mul(boughtERC20Amount).div(SWAP_FEE_DIVISOR);

// uint256 toTransfer = (SWAP_FEE_DIVISOR - swapFee) * boughtERC20Amount / SWAP_FEE_DIVISOR;

// same with many other computations

Handle

cmichel

Vulnerability details

The minimumAmountReceived check in Swap.swapByQuote is implemented like this:

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

It can be simplified to this which performs less calls to signifiesETHOrZero and less logical operators:

require( (signifiesETHOrZero(zrxBuyTokenAddress) ? boughtETHAmount : boughtERC20Amount) >= minimumAmountReceived, "...");