Rubicon contest - Chom's results

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconMarket.sol#L471-L473

Vulnerability details

Impact

isClosed() always return false, which means no one can close the market. If it shouldn't be closed, why this function exists.

isClosed is being used in can_offer, can_buy and can_cancel modifier.

Proof of Concept

isClosed() always return false as seen below.

    function isClosed() public pure returns (bool closed) {
        return false;
    }

Moreover, isClosed is not overridden in any other lines.

Tools Used

Scan code by eye

Recommended Mitigation Steps

Change isClosed to boolean variable and let operator/owner able to assign this variable, guarded by some kind of Timelock.

• Core contracts are written in solidity >=0.6.0 <0.8.0 which is outdated.
• You should use solidity 0.8.0, so that you can avoid using SafeMath while overflow/underflow problem is also prevented.
• You should replace transfer with safeTransfer in SafeERC20 library to prevent non-standard token from ruining your contract.
• It would be better if you use hardhat since it coming with Proxy helper tools to deploy and upgrade Proxy in standard way.
• You shouldn't copy standard contracts like ERC20, IERC20 to your project. Instead, import directly from openzeppelin.

• Change to solidity 0.8.x and avoid using SafeMath. Native solidity safe math engine is better optimized.
• Use unchecked block on all places where overflow/underflow never happen or has already checked.