Rubicon contest - GimelSec's results

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L35 6 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L374 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L434 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L451 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L491 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L548

Vulnerability details

Impact

Transfer ETH by using transfer() may cause this transaction to fail. In EIP-1884:

In many cases, a recipient of ether from a CALL will want to issue a LOG. The LOG operation costs 375 plus 375 per topic. If the LOG also wants to do an SLOAD, this change may make some such transfers fail.

Proof of Concept

RubiconRouter.sol:356:            msg.sender.transfer(delta);
RubiconRouter.sol:374:        msg.sender.transfer(buy_amt); // Return native ETH
RubiconRouter.sol:434:            msg.sender.transfer(delta);
RubiconRouter.sol:451:        msg.sender.transfer(pay_amt);
RubiconRouter.sol:491:        msg.sender.transfer(withdrawnWETH);
RubiconRouter.sol:548:        msg.sender.transfer(fill);

Tools Used

None

Recommended Mitigation Steps

Use call{value: amount}() instead of transfer:

    (bool success, ) = payable(msg.sender).call{value: amount}("");
    require(success, "Address: unable to send value, recipient may have reverted");

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L251 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L303 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L320 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L348 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L377 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L406 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L471

Vulnerability details

Impact

Some ERC20 returns false rather than reverting when transfer fails. This will cause the transaction to execute successfully but users don’t successfully get the token.

Proof of Concept

In RubiconRouter.sol, users may lose tokens if transferring tokens to users without check return value:

RubiconRouter.sol:251:            ERC20(route[route.length - 1]).transfer(to, currentAmount);
RubiconRouter.sol:303:        ERC20(buy_gem).transfer(msg.sender, fill);
RubiconRouter.sol:320:        ERC20(buy_gem).transfer(msg.sender, fill);
RubiconRouter.sol:348:        ERC20(buy_gem).transfer(msg.sender, buy_amt);
RubiconRouter.sol:377:            ERC20(pay_gem).transfer(msg.sender, max_fill_amount - fill);
RubiconRouter.sol:406:            ERC20(buy_gem).transfer(msg.sender, _after - _before);
RubiconRouter.sol:471:        ERC20(targetPool).transfer(msg.sender, newShares);

Other transfer should check:

RubiconRouter.sol:202:        ERC20(route[0]).transferFrom(
RubiconRouter.sol:274:        ERC20(route[0]).transferFrom(
RubiconRouter.sol:366:        ERC20(pay_gem).transferFrom(msg.sender, address(this), max_fill_amount); //transfer pay here
RubiconRouter.sol:419:        ERC20(pay_gem).transferFrom(msg.sender, address(this), pay_amt);
RubiconRouter.sol:486:        IBathToken(targetPool).transferFrom(msg.sender, address(this), shares);
peripheral_contracts/BathBuddy.sol:114:            token.transfer(recipient, amountWithdrawn);
rubiconPools/BathPair.sol:601:            IERC20(asset).transfer(msg.sender, booty);
rubiconPools/BathPair.sol:615:            IERC20(quote).transfer(msg.sender, booty);
rubiconPools/BathToken.sol:353:        IERC20(filledAssetToRebalance).transfer(
rubiconPools/BathToken.sol:602:            underlyingToken.transfer(feeTo, _fee);
rubiconPools/BathToken.sol:605:        underlyingToken.transfer(receiver, amountWithdrawn);

Tools Used

None

Recommended Mitigation Steps

Use SafeERC20: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

  using SafeERC20 for IERC20;

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L462 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L504-L507

Vulnerability details

Impact

If a user sends more ETH than the user has to, the contract just accepts it. The user will lose more ETH accidentally.

Proof of Concept

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

        require(msg.value >= amount, "didnt send enough eth");

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L504-L507

        require(
            msg.value >= amtWithFee,
            "must send enough native ETH to pay as weth and account for fee"
        );

Tools Used

None

Recommended Mitigation Steps

Use == rather than >=, or refund the remaining ETH.

Lines of code

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

Vulnerability details

Impact

Some ERC20 tokens have a fee on each transfer. The protocol doesn’t handle the fee when transferring this kind of ERC20 tokens, leading to the inconsistent amount of token actually received in the contract. This will cause users to fail to execute functions due to not enough tokens in the protocol, or the protocol will lose more tokens to compensate users for the execution.

Proof of Concept

In the offer function, the amount of the token is recorded in pay_amt. However, if the ERC20(pay_gem) collects a fee on every transfer. The actual amount of token received is not equal to pay_amt. info.pay_amt = pay_amt; will record more amount of token than it received.

392     function offer(
    ...
407         info.pay_amt = pay_amt;
    ...
416         require(pay_gem.transferFrom(msg.sender, address(this), pay_amt));

Tools Used

None

Recommended Mitigation Steps

Use balanceAfter - balanceBefore rather than using amount directly:

function retrieveTokens(address sender, uint256 amount) public {
    uint256 balanceBefore = deflationaryToken.balanceOf(address(this));
    deflationaryToken.transferFrom(sender, address(this), amount);
    uint256 balanceAfter = deflationaryToken.balanceOf(address(this));

    amount = balanceAfter.sub(balanceBefore);
}

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L156-L168

Vulnerability details

Impact

It will be broken if users try to call openBathTokenSpawnAndSignal on rubiconPools/BathHouse.sol.

Proof of Concept

Users can call openBathTokenSpawnAndSignal with parameters newBathTokenUnderlying and initialLiquidityNew:

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L156-L168

        require(
            newBathTokenUnderlying.transferFrom(
                msg.sender,
                address(this),
                initialLiquidityNew
            ),
            "Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol"
        );

        newBathTokenUnderlying.approve(newOne, initialLiquidityNew);

        // Deposit assets and send Bath Token shares to msg.sender
        IBathToken(newOne).deposit(initialLiquidityNew, msg.sender);

In deposit of BathToken.sol, it will call underlyingToken transfer:

        underlyingToken.transferFrom(msg.sender, address(this), assets);

If the underlyingToken is a fee-on-transfer token, it will revert because assets are not enough.

Tools Used

None

Recommended Mitigation Steps

Use balanceAfter - balanceBefore rather than using amount directly:

function retrieveTokens(address sender, uint256 amount) public {
    uint256 balanceBefore = deflationaryToken.balanceOf(address(this));
    deflationaryToken.transferFrom(sender, address(this), amount);
    uint256 balanceAfter = deflationaryToken.balanceOf(address(this));

    amount = balanceAfter.sub(balanceBefore);
}

Lines of code

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

Vulnerability details

Impact

The feeBPS is unlimited in RubiconMarket.sol, leading to maliciously moving large taxes from this contract. Because the fee is taken separately, the user may lose all tokens owned by the user.

Proof of Concept

There is no cap on feeBPS

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

    function setFeeBPS(uint256 _newFeeBPS) external auth returns (bool) {
        feeBPS = _newFeeBPS;
        return true;
    }

When collecting protocol fee, unlimited feeBPS could lead to maliciously obtaining large amounts of token from users

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

        // Fee logic added on taker trades
        uint256 fee = mul(spend, feeBPS) / 10000;
        require(
            _offer.buy_gem.transferFrom(msg.sender, feeTo, fee),
            "Insufficient funds to cover fee"
        );

If Alice has a large amount of token, and try to buy an offer from the protocol. A malicious admin can front-run the transaction, set a large feeBPS to take all tokens owned by alice.

Tools Used

None

Recommended Mitigation Steps

There should be a reasonable cap on feeBPS. (e.g., 20%)

Lines of code

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

Vulnerability details

Impact

A malicious admin can set the deprecated variables AqueductDistributionLive and AqueductAddress to deny specific users to buy.

Proof of Concept

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

        if (AqueductDistributionLive) {
            IAqueduct(AqueductAddress).distributeToMakerAndTaker(
                getOwner(id),
                msg.sender
            );
        }

Admin can set the deprecated variables AqueductDistributionLive, AqueductAddress to check specific users and revert transactions to deny them.

Tools Used

None

Recommended Mitigation Steps

Delete deprecated code, but reserve variable declaration for slots if using upgradable contracts.

(Low) Use multisig wallet or DAO as admin

Impact

In document:

Importantly, BathHouse has an admin that is the EOA administrator of the entire protocol in v1.

It’s better to use multisig wallet or DAO as admin.

Proof of Concept

https://github.com/code-423n4/2022-05-rubicon#bathhousesol

Tools Used

None

Recommended Mitigation Steps

Use a multisig wallet such as Gnosis Safe.

Save gas in for loops by ++i rather than i++

In for loops, using ++i rather than i++ to save gas.

Proof of Concept

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

rubiconPools/BathPair.sol
311:        for (uint256 index = 0; index < array.length; index++) {
427:        for (uint256 index = 0; index < quantity; index++) {
480:        for (uint256 index = 0; index < quantity; index++) {
582:        for (uint256 index = 0; index < ids.length; index++) {

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

Recommendation

Use ++i rather than i++ to save gas.