Rubicon contest - z3s's results

Judge has assessed an item in Issue #408 as Medium risk. The relevant finding follows:

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L361

Vulnerability details

Impact

    function cancel(uint256 id)
        public
        virtual
        can_cancel(id)
        synchronized
        returns (bool success)
    {
        OfferInfo memory _offer = offers[id];
        delete offers[id];

        require(_offer.pay_gem.transfer(_offer.owner, _offer.pay_amt)); // @audit non-standard compliant tokens like USDT

        emit LogItemUpdate(id);
        emit LogKill(
            bytes32(id),
            keccak256(abi.encodePacked(_offer.pay_gem, _offer.buy_gem)),
            _offer.owner,
            _offer.pay_gem,
            _offer.buy_gem,
            uint128(_offer.pay_amt),
            uint128(_offer.buy_amt),
            uint64(block.timestamp)
        );

        success = true;
    }

if _offer.pay_gem === <USDT_ADDRESS> this function will revert because transfer of usdt does not return anything. and ERC20.transfer want a boolean. so the cancel will never happen if _offer.pay_gem === <USDT_ADDRESS>.

Recommended Mitigation Steps

Use OpenZeppelin’s SafeERC20 safeTransfer/safeTransferFrom instead of transfer/transferFrom that handles the return value check as well as non-standard-compliant tokens.

Gas Optimizations

[G01] uint256 default value is 0 so we can remove = 0:

RubiconMarket.sol
  990,25:         uint256 old_top = 0;

RubiconRouter.sol
  82,25:         uint256 lastBid = 0;
  83,25:         uint256 lastAsk = 0;
  85,28:         for (uint256 index = 0; index < topNOrders; index++) {
  168,31:         uint256 currentAmount = 0;
  169,24:         for (uint256 i = 0; i < route.length - 1; i++) {
  226,31:         uint256 currentAmount = 0;
  227,24:         for (uint256 i = 0; i < route.length - 1; i++) {

rubiconPools/BathToken.sol
  635,32:             for (uint256 index = 0; index < bonusTokens.length; index++) {

[G02] ++i use less gas than i++:

++i costs less gas compared to i++. about 5 gas per iteration.

RubiconRouter.sol
  169,51:         for (uint256 i = 0; i < route.length - 1; i++) {
  227,51:         for (uint256 i = 0; i < route.length - 1; i++) {