Rubicon contest - Metatron's results

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L655-L661 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L868-L874 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L678-L684 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L697-L701 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L713-L714 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L829-L835 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L612-L618 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L225-L230

Vulnerability details

Impact

Core functionality of the core contract RubiconMarket.sol ("Rubicon revolves around a core smart contract RubiconMarket.sol") is not re-entrance safe. Including: offer, buy, cancel, insert, del_rank, sellAllAmount, buyAllAmount. They do not grab the lock locked or releasing it at any point, therefore the require(!locked, "Reentrancy attempt") statement in all the functions above will always pass and no re-entrance guard available.

Proof of Concept

An inheriting contract that overrides a function, will not apply the original modifiers automatically. Given the code snippet below, calling buy on SMarket with q other then 0 will result in re-entrance and will revert. On the contrast, calling buy on RMarket with any q will call buy repeatedly q-1 times - ignoring the locked boolean state, allowing re-entrance and can result in stolen funds or rendering the system useless.

pragma solidity =0.7.6;

contract SMarket {
    bool locked = false;
    
    modifier synchronized() {
        require(!locked, "reentrece detected - lock already taken");
        locked = true;
        _;
        locked = false;
    }

    function buy(uint256 q) public virtual synchronized returns (bool) {
        if (q > 0) { 
            return buy(q-1); 
        }
        else { 
            return true; 
        }
    }
}

contract RMarket is SMarket {
    function buy(uint256 q) public override returns (bool) {
        if (q > 0) {
            return buy(q-1);
        }
        else {
            return true;
        }
    }
}

Tools Used

Remix

Recommended Mitigation Steps

Add the synchronized modifier to all 7 functions stated, to prevent re-entrance risk.

--This is the updated gas report-- I've sent a partial report by mistake, few minutes ago. I know we should not send multiple gas reports.. will not happen again.

[G-01] An array’s length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration. I suggest storing the array’s length in a variable before the for-loop, in all of the following places: https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L635 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L311 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L582

[G-02] ++i costs less gas compared to i++

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration) I suggest using ++i instead of i++ to increment the value of an uint variable, in all of the following places: https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L206 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L311 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L427 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L480 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L582 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L635 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L85 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L169 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L227 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L436 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L1165

[G-03] In BathToken.sol, can save 22 gas without loosing readability by assigning value at variable declaration https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L489-L502 Change L497-L499: uint256 amountWithdrawn; uint256 _fee = assets.mul(feeBPS).div(10000); amountWithdrawn = assets.sub(_fee); To: uint256 _fee = assets.mul(feeBPS).div(10000); uint256 amountWithdrawn = assets.sub(_fee);

[G-04] In ERC20.sol, _name and _symbol will not change after the the constructor, use immutable variable instead of storage variable can save gas. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/ERC20.sol#L42-L44

[G-05] In ERC20.sol, If you set _decimals directly in the constractor, instead of in _setupDecimals - which is only called at the constructor - you can make it immutable to save gas, and you also save a function call. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/ERC20.sol#L62