Rubicon contest - kenzo's results

Lines of code

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

Vulnerability details

Anybody can cancel an offer made by [another user using RoubiconRouter].

Impact

If a user created an offer which wasn't fully filled, anybody (including bots) can swoop in, cancel it, and get the user's funds.

Proof of Concept

Anybody can create an offer using offerForETH. The router does not save the offer's originator. Then by calling 'cancelForETH', which does not check who called it, anybody can sweep the user's funds.

Recommended Mitigation Steps

Few options are there -

• Consider saving in the router the offer's originator - although that ruins the router's statelessness
• Consider adding a parameter to RubiconMarket's offer function (\overload) that will allow passing who can cancel the offer
• Consider removing the functionality altogether
• Consider adding a comment warning the user of the risk of using this functionality
• Consider the futility of constantly trying to chase happiness

Lines of code

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

Vulnerability details

When swapping amongst multiple pairs in RubiconRouter's _swap, the fee is wrongly accounted for.

Impact

Not all of the user's funds would be forwarded to RubinconMarket, therefore the user would lose funds.

Proof of Concept

The _swap function is calculating the pay amount to send to RubiconMarket.sellAllAmount to be:

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

But this would lead to not all of the funds being pulled by RubiconMarket. I mathematically show this in this image. The correct parameter that needs to be sent to sellAllAmount is:

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

I mathematically prove this in this image.

Recommended Mitigation Steps

Change the parameter to the abovementioned one.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L356 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

Throughout RubiconRouter, ETH is being sent to the sender using .transfer. This only forwards 2300 gas back to the caller. This is generally not recommended.

Impact

This makes it problematic (workarounds are needed) to transfer funds to wallets such as Gnosis, and other flows which require doing actions (like invest the funds) upon receival will be broken.

Proof of Concept

See links above and Consensys article linked above.

Recommended Mitigation Steps

Use .call.value(...)("") to send ETH.

Lines of code

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

Vulnerability details

Some tokens, like USDT, do not return a value upon successful transfer. Since Rubicon is checking that the transfer succeeded by checking the return value, these tokens can not be used. This is why SafeERC20 is generally used for transfers.

Impact

Some tokens, like the popular USDT, can not be used.

Proof of Concept

We can see in USDT's code that it's transfer function does not return anything. When Rubicon creates a pair, it requires that the transfer will return true:

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

Therefore, the require will fail for certain tokens. There are more instances of simple transfer throughout the protocol - all of those need to be changed to use safeTransfer.

Recommended Mitigation Steps

Add SafeERC20 to the protocol and change all transfers to safeTransfer.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L180 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L157 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L256

Vulnerability details

When using the approval mechanism in USDT, the approval must be set to 0 before it is updated. In Rubicon, when creating a pair, the paired asset's approval is not set to 0 before it is updated.

Impact

Can't create pairs with USDT, the most popular stablecoin, as as the approval will revert.

Proof of Concept

USDT reverts on approval if previous allowance is not 0:

require(!((_value != 0) && (allowed[msg.sender][_spender] != 0)));

When creating a pair, Rubicon approves the paired asset without first setting it to 0:

desiredPairedAsset.approve(pairedPool, initialLiquidityExistingBathToken);

Therefore, if desiredPairedAsset is USDT, the function will revert, and pairs with USDT can not be created.

This problem will also manifest in RubiconMarket's approval function and BathToken's approval function,

Recommended Mitigation Steps

Set the allowance to 0 before setting it to the new value.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L499

Vulnerability details

The fee is wrongly accounted for in previewWithdraw.

Impact

Function returns wrong result; Additionally, withdraw(assets,to,from) will always revert. (The user can still withdraw his assets via other functions).

Proof of Concept

The previewWithdraw function returns less shares than the required assets (notice the substraction):

            uint256 amountWithdrawn;
            uint256 _fee = assets.mul(feeBPS).div(10000);
            amountWithdrawn = assets.sub(_fee);
            shares = convertToShares(amountWithdrawn);

This won't work, because if the user wants to receive amount of assets, he needs to burn more shares than that to account for the fee. Not less. This will also make withdraw(assets,to,from) revert, because it takes the amount of shares from previewWithdraw, and then checks how much assets were really sent to the user, and verifies that it's at least how much he asked for:

        uint256 expectedShares = previewWithdraw(assets);
        uint256 assetsReceived = _withdraw(expectedShares, receiver);
        require(assetsReceived >= assets, "You cannot withdraw the amount of assets you expected");

But since the expectedShares is smaller than the original amount, and since _withdraw deducts the fee from expectedShares, then always assets > assetsReceived, and the function will revert.

Recommended Mitigation Steps

The amount of shares that previewWithdraw should return is: convertToShares(assets.add(assets.mul(feeBPS).div((10000.sub(feeBPS)))) I prove this mathematically in this image.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L261

Vulnerability details

The fee BPS parameter is not capped.

Impact

Admin can set fee to be 10000 and thereby transfer all deposits to himself. While admin is a trusted role, some sanity guarantees should probably be in place.

Proof of Concept

Well, the fee BPS is not capped.

    function setFeeBPS(uint256 _feeBPS) external onlyBathHouse {
        feeBPS = _feeBPS;
    }

Recommended Mitigation Steps

Cap it to a reasonable amount such as 1000.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L591:#L625

Vulnerability details

When calling strategistBootyClaim, the strategist can pass same the token address twice. The function doesn't check for this, and uses normal subtraction instead of SafeMath.

Impact

Strategist can claim more rewards from BathPair than he deserves. Although the strategist is a trusted role, it is planned to become permisionless, and anyway such logic bugs should not exist in the system.

Proof of Concept

The function starts by locally saving the amount of rewards for each token:

    function strategistBootyClaim(address asset, address quote)
        external
        onlyApprovedStrategist(msg.sender)
    {
        uint256 fillCountA = strategist2Fills[msg.sender][asset];
        uint256 fillCountQ = strategist2Fills[msg.sender][quote];

Note that it does not check whether asset != quote. It then proceeds to send the rewards for the asset token:

        if (fillCountA > 0) {
            uint256 booty = (fillCountA.mul(IERC20(asset).balanceOf(address(this)))).div(totalFillsPerAsset[asset]);
            IERC20(asset).transfer(msg.sender, booty);
            totalFillsPerAsset[asset] -= fillCountA;
            strategist2Fills[msg.sender][asset] -= fillCountA;
        }

And then does the same for the quote token:

        if (fillCountQ > 0) {
            uint256 booty = (fillCountQ.mul(IERC20(quote).balanceOf(address(this)))).div(totalFillsPerAsset[quote]);
            IERC20(quote).transfer(msg.sender, booty);
            totalFillsPerAsset[quote] -= fillCountQ;
            strategist2Fills[msg.sender][quote] -= fillCountQ;
        }

So what would happen at this stage if asset == quote?

• fillCountQ is still the original reward amount, as it was saved locally. So it is bigger than 0.
• If there are rewards of other strategists in the contract, the contract balance and totalFillsPerAsset are bigger than 0, so booty would be sent.
• Both the substractions at the end of the block are not using SafeMath - therefore, they would underflow and not revert like they should.

Recommended Mitigation Steps

I believe changing the last lines to use SafeMath, meaning use .sub instead of -, would be enough to mitigate the bug. You can also add sanity checks like asset != quote but it might not be necessary.

Lines of code

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

Vulnerability details

There is no option to revoke strategist's privilege. As the strategist is a very strategic role which can effectively steal LP's funds, this is very dangerous.

Impact

A rogue / compromised / cancelled strategist can not be revoked of permissions.

Proof of Concept

There's a function to approve a strategist, but no option to revoke the access.

Recommended Mitigation Steps

Add a function / change the function and allow setting strategist's access to false.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/BathBuddy.sol#L114:#L116

Vulnerability details

BathBuddy's release function is not protected from reentrancy.

Impact

User can withdraw more rewards than he deserves, in tokens with a receiver hook like ERC777.

Proof of Concept

• User will withdraw half his assets, thereby initiating distributeBonusTokenRewards which will call rewardsVestingWallet.release.
• (Here's the problem) The release function sends the tokens to the user before updating the contract state:

            token.transfer(recipient, amountWithdrawn);
            _erc20Released[address(token)] += amount;

• Upon receival of tokens, the user will call withdraw again with the rest of his assets.
• When execution will arrive to BathBuddy's release again, it will deduct the amount already released from the releaseable (released == _erc20Released):

        uint256 releasable = vestedAmount(address(token), uint64(block.timestamp)) - released(address(token));

Therefore, since _erc20Released was not updated yet, the releasable will be larger than it really is, and the user will receive more awards than deserved.

Recommended Mitigation Steps

Conform with Checks-Effects-Interactions pattern and send the tokens only after updating the state.