Rubicon contest - PP1004's results

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/BathBuddy.sol#L87-L128

Vulnerability details

Impact

• User can claim more reward than expected by call deposit -> withdraw many times
• Make other users lose their reward

Proof of Concept

BathBuddy just divides the portion of rewards based on BathToken's shares. But it doesn't care about how long that user has deposited to the BathToken. It will lead to the scenario that the user can call deposit and withdraw many times to claim more rewards.

For more detail pls read and add this file to your test folder and run it (also note that add one more function setRewardsVestingWallet() that I note in the file for easy to test). I described all the detail of the issue through a simple example.

Tools Used

javascript, truffle

Recommended Mitigation Steps

The logic of BathBuddy should follow the logic of this one

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L557-L585 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L756-L759

Vulnerability details

Impact

The first user can claim all the rewards, and maybe no1 can join the pool

Proof of Concept

Rich user can manipulate the deposit function follow these steps: Step 1: Deposit 1 underlyingtoken to BathToken and get 1 share. Step 2: Execute a direct transfer 1M underlyingToken to BathToken ==> After this step, 1 share of BathToken will cost (1M + 1) underlyingToken. Assume underlyingToken is USDC, so it will make the share too expensive for normal (poor) people to join (they will need at least 1M + 1 USDC to get 1 share ...). ==> User can claim all of the rewards when he is the first one joins the pool

Tools Used

manual review

Recommended Mitigation Steps

Instead of calculating underlyingBalance() by IERC20(underlyingToken).balanceOf(address(this)), we can create a variable totalUnderlyingToken that will be increased anytime someone call deposit() function.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L80

Vulnerability details

Impact

• Make BathBuddy contract useless
• Protocol is not working as expected
• Fewer incentives for users to join BathToken

Proof of Concept

variable rewardsVestingWallet in BathToken is used to store BathBuddy contract. BathBuddy make it easy for Bath Tokens to payout their withdrawers any bonusToken they may have accrued while staking in Bath Token (incentives). But in contract BathToken.sol, there is no function to initialize/set variable rewardVestingWallet.

Tools Used

manual review

Recommended Mitigation Steps

Add one more function to let BathHouse set variable rewardsVestingWallet.

function setRewardsVestingWallet(IBathBuddy newRewardsVestingWallet) onlyBathHouse {
    rewardsVestingWallet = newRewardsVestingWallet;
}

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L232

Vulnerability details

Impact

The wrong fee calculation can cause a loss to users' fund and this loss will be stuck in RubiconRouter

Proof of Concept

We have the default $feeBPS = 20, BPS = 10000$

Let's assume that alice call RubiconRouter.swap(pay_amt=1000000)

Through router, alice will have to transfer in:

$amountTransferIn = 1000000 \times (BPS + feeBPS) / BPS = 1002000$

However in the internal _swap function, the actual amount being pass into RubiconMarket.sellAllAmount() is

$amountTransferIn - amountTransferIn \times feeBPS / BPS = 999996$

So in this case, $4$ over $1002000$ unit of user's fund were stuck in the router and not being used to swap.

Recommended Mitigation Steps

The formula in L232-L234 in Rubicon Router should be changed to:

    currentAmount.sub(
        currentAmount.mul(expectedMarketFeeBPS).div(expectedMarketFeeBPS + 10000)
    )

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L356 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L374 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L434 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L451 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L491 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L548

Vulnerability details

Impact

User can't claim their native token back

Proof of Concept

To withdraw native token using transfer() will fail inevitably when :

• The withdrawer smart contract does not implement a payable function.
• Withdrawer's smart contract does implement a payable fallback which uses more than 2300 gas unit
• Withdrawer's smart contract implements a payable fallback function which needs less than 2300 gas unit but is called through proxy that raise the call's gas usage above 2300

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

Manual review

Recommended Mitigation Steps

use call() to send eth (native token)

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L353-L357

Vulnerability details

Impact

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

While most places use a require or safeTransfer/safeTransferFrom, there are three missing cases in the withdrawal of staking token and rescue of arbitrary tokens sent to the FeeDistributor contract.

Reference this similar medium-severity finding from Consensys Diligence Audit of Fei Protocol: https://consensys.net/diligence/audits/2021/01/fei-protocol/#unchecked-return-value-for-iweth-transfer-call

Proof of Concept

• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L353-L357
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L601-L603
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L605
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L565
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L601
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L615
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/BathBuddy.sol#L114
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L202-L206
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L251
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L274-L278
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L303
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L320
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L348
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L366
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L377
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L406
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L419
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L471
• https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L486

Tools Used

manual review

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

Lines of code

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

Vulnerability details

Impact

Different between amount of pay_gem that offer actual has and pay_amt ==> User can't buy that offer because there is not enough fund to transfer

Proof of Concept

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom()

Assume that pay_gem is Deflationary token, so after line 416, the actual amount that contract gained is less than buy_amt, because pay_gem will take a fee on trasferFrom() function.

Tools Used

manual review

Recommended Mitigation Steps

set value of pay_amt equal to the difference between of pay_gem after and before transferFrom() function.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathHouse.sol#L136-L203

Vulnerability details

Impact

Function openBathTokenSpawnAndSignal will alway revert when newBathTokenUnderlying or desiredPairedAsset is deflationary token

Proof of Concept

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom() For example, I will assume that newBathTokenUnderlying is deflationary token. After line 163, the actual amount of newBathTokenUnderlying that BathHouse gained will be smaller than initialLiquidityNew. It will make the deposit call reverted because there are not enough fund to transfer.

Tools Used

Manual review

Recommended Mitigation Steps

set initialLiquidityNew = newBathTokenUnderlying.balanceOf(address(this)) after line 163 and initialLiquidityExistingBathToken = desiredPairedAsset.balanceOf(address(this)) after line 178

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L505-L521 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L489-L502

Vulnerability details

Impact

Function previewWithdraw() return the wrong amount of shares needed to burn to withdraw a given asset. It can make function withdraw(uint256 assets, address receiver, address owner) invalid to use.

Proof of Concept

For easy to describe the issue, I assume that feeBps = 0. For example we have 1000 underlyingToken and 100 shares. If we want to withdraw 9 underlyingToken, we will need to burn at least shares = assets * totalSupply / totalAssets() = 9 * 100 / 1000 = 0 ==> So we will need to burn 0 shares to get 9 tokens ? (make no sense). We can call withdraw(9, receiver, owner) for test, the function will revert because assetsReceived is equal to 0 < 9

Tools Used

manual review

Recommended Mitigation Steps

Instead of using function convertToShare in function previewWithdraw, we can use the following formula instead

function ceil(uint a, uint m) constant returns (uint ) {
    return ((a + m - 1) / m) * m;
}
function previewWithdraw(uint256 assets)
        public
        view
        returns (uint256 shares)
    {
        if (totalSupply == 0) {
            shares = 0;
        } else {
            uint256 amountWithdrawn;
            uint256 _fee = assets.mul(feeBPS).div(10000);
            amountWithdrawn = assets.sub(_fee);
            (totalSupply == 0) ? shares = assets : shares = ceil(
                assets.mul(totalSupply).div(totalAssets()));
        }
    }

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L307

Vulnerability details

Impact

The RubiconRouter function maxSellAllAmount does not transfer user's fund into its address, causing the function to always revert

Proof of Concept

Since there is no fund transferred into router during the maxSellAllAmount call, it will always revert when RubiconMarket tries to take tokens from it.

Recommended Mitigation Steps

Add a transfer of fund to the function

    /// @dev this function takes a user's entire balance for the trade in case they want to do a max trade so there's no leftover dust
    function maxSellAllAmount(
        ERC20 pay_gem,
        ERC20 buy_gem,
        uint256 min_fill_amount
    ) external returns (uint256 fill) {
        //swaps msg.sender entire balance in the trade
        uint256 maxAmount = ERC20(buy_gem).balanceOf(msg.sender);
        ERC20(buy_gem).safeTransferFrom(msg.sender, address(this), maxAmount);
        fill = RubiconMarket(RubiconMarketAddress).sellAllAmount(
            pay_gem,
            maxAmount,
            buy_gem,
            min_fill_amount
        );
        ERC20(buy_gem).transfer(msg.sender, fill);
    }

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L290 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/RubiconRouter.sol#L307

Vulnerability details

Impact

The two functions maxSellAllAmount and maxBuyAllAmount will always revert in case at least (100-fee)% of user's balance can be matched with orders.

Proof of Concept

Let say Bob placed an order selling 100 USDC with a low USDT price of 1:0.95.

Alice currently has 50 USDT and they want to maxSellAllAmount into USDC.

The function will pass 50 as amount into RubiconMarket's buyAll function where it fully matches with Bob's order. Here, the buy() function will first transfer alice's 50 USDT in and later 50 * feeBPS / BPS as fee. In this case, alice can not afford to pay.

Therefore, the two functions maxSellAllAmount and maxBuyAllAmount are useless in case user's request can be fully matched.

Recommended Mitigation Steps

Add the fee calculating before passing the amount to the RubiconMarket's buyAll, sellAll function.

    /// @dev this function takes a user's entire balance for the trade in case they want to do a max trade so there's no leftover dust
    function maxBuyAllAmount(
        ERC20 buy_gem,
        ERC20 pay_gem,
        uint256 max_fill_amount
    ) external returns (uint256 fill) {
        //swaps msg.sender's entire balance in the trade   
        
        uint256 maxAmount = _calcAmountAfterFee(ERC20(buy_gem).balanceOf(msg.sender));
        
        fill = RubiconMarket(RubiconMarketAddress).buyAllAmount(
            buy_gem,
            maxAmount,
            pay_gem,
            max_fill_amount
        );
        ERC20(buy_gem).transfer(msg.sender, fill);
    }

    /// @dev this function takes a user's entire balance for the trade in case they want to do a max trade so there's no leftover dust
    function maxSellAllAmount(
        ERC20 pay_gem,
        ERC20 buy_gem,
        uint256 min_fill_amount
    ) external returns (uint256 fill) {
        //swaps msg.sender entire balance in the trade

        uint256 maxAmount = _calcAmountAfterFee(ERC20(buy_gem).balanceOf(msg.sender));
        fill = RubiconMarket(RubiconMarketAddress).sellAllAmount(
            pay_gem,
            maxAmount,
            buy_gem,
            min_fill_amount
        );
        ERC20(buy_gem).transfer(msg.sender, fill);
    }


    function _calcAmountAfterFee(uint256 amount) internal view returns (uint256) {
        uint256 feeBPS = RubiconMarket(RubiconMarketAddress).getFeeBPS();
        return amount.sub(amount.mul(feeBPS).div(10000));
    }

QA

BathHouse.sol#L132

There is a typo in this comment.

gaurentee -> guarantee

BathHouse.sol#L285

The comment for this function is wrong

    /// @notice Admin-only function to set a Bath Token's timeDelay
    function setBathTokenMarket(address bathToken, address newMarket)
        external
        onlyAdmin
    {
        IBathToken(bathToken).setMarket(newMarket);
    }

BathHouse.sol#L382-L384 (Gas op)

Remove redundent declaration for gas saving

    function isApprovedPair(address pair) public view returns (bool) {
        return pair == approvedPairContract;
    }

BathPair.sol#L122

Should use address(0) here for readability

BathPair.sol#L206-L207 (Gas op)

Can return ++last_stratTrade_id to save on read from storage.

BathPair.sol#L230-L247 (Gas op)

This part of code can be shortenned to

if (info.askId != 0) {
    // if delta > 0 - delta is fill => handle any amount of fill here
    if (askDelta > 0) {
        logFill(askDelta, info.strategist, info.askAsset);
        IBathToken(bathAssetAddress).removeFilledTradeAmount(askDelta);
    }
    if (offer1.pay_amt) {
        IBathToken(bathAssetAddress).cancel(
            info.askId,
            offer1.pay_amt
        );
    }
}

to save gas

BathPair.sol#L250-L267

Same as above comment

BathPair.sol#L303-L319

Since this function is only used for removing a single matching element from the array.

It is better to just make it remove the element inside itself to save gas

    function removeElement(uint256 uid, uint256[] storage array)
        internal
        view
    {
        for (uint256 index = 0; index < array.length; index++) {
            if (uid == array[index]) {
                array[index] = array[array.length - 1];
                return;
            }
        }
        revert("Didnt Find that element in live list, cannot scrub");
    }

BathToken.sol#L346

There should be a check for filledAssetToRebalance != underlying here to revert on malicious case.