Rubicon contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

The swapEntireBalance() function allows the user to pass a buy_amt_min value which is the minimum number of tokens they should receive from the swap. But, the function doesn't pass the value to the underlying swap() function. Thus, the user's min value will be ignored. Since that will result in unexpected outcomes where user funds might be lost, I rate this issue as HIGH.

Proof of Concept

swapEntireBalance():

    function swapEntireBalance(
        uint256 buy_amt_min,
        address[] calldata route, // First address is what is being payed, Last address is what is being bought
        uint256 expectedMarketFeeBPS
    ) external returns (uint256) {
        //swaps msg.sender entire balance in the trade
        uint256 maxAmount = ERC20(route[0]).balanceOf(msg.sender);
        ERC20(route[0]).transferFrom(
            msg.sender,
            address(this),
            maxAmount // Account for expected fee
        );
        return
            _swap(
                maxAmount,
                maxAmount.sub(buy_amt_min.mul(expectedMarketFeeBPS).div(10000)), //account for fee
                route,
                expectedMarketFeeBPS,
                msg.sender
            );
    }

The second parameter of the _swap() call should be the min out value. Instead maxAmount.sub(buy_amt_min.mul(expectedMarketFeeBPS).div(10000)) is used.

Example:

amount = 100
buy_amt_min = 99
expectedMarketFeeBPS = 500 // 5%

actual buy_amy_min = 100 - (99 * (500 / 10000)) = 95.05

So instead of using 99 the function uses 95.05 which could result in the user receiving fewer tokens than they expected.

Tools Used

none

Recommended Mitigation Steps

pass buy_amt_min directly to _swap()

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

Impact

When transferring ETH, use call() instead of transfer().

The transfer() function only allows the recipient to use 2300 gas. If the recipient uses more than that, transfers will fail. In the future gas costs might change increasing the likelihood of that happening.

Keep in mind that call() introduces the risk of reentrancy. But, as long as the router follows the checks effects interactions pattern it should be fine. It's not supposed to hold any tokens anyway.

Proof of Concept

See the linked code snippets above.

Tools Used

none

Recommended Mitigation Steps

Replace transfer() calls with call(). Keep in mind to check whether the call was successful by validating the return value:

(bool success, ) = msg.sender.call{value: amount}("");
require(success, "Transfer failed.")

Lines of code

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

Vulnerability details

Impact

Many ERC20 tokens don't implement the specification correctly. Some of them revert and some just return false when there's an issue. Some of them don't return anything. It's the wild west. When your contract is expected to work with a wide range of tokens you should use OpenZeppelin's SafeERC20 library. Otherwise, you might run into an issue where some tokens don't work with your protocol or transfers fail without the contract even knowing. The latter can result in lost funds so I rate it as high. Especially considering ERC20 trades is the main feature of this protocol.

Proof of Concept

A good example is the USDT token. When a transfer succeeds, it doesn't return true: https://etherscan.io/address/0xdac17f958d2ee523a2206206994597c13d831ec7#code#L126

So if you have the following check in your contract, it will always fail with the USDT contract:

require(pay_gem.transferFrom(msg.sender, address(this), pay_amt));

Another one is the 0x token which returns false when a transfer wasn't successful. So if you have instances where you don't check the return value, you might expect funds to be transferred although it failed: https://etherscan.io/address/0xe41d2489571d322189246dafa5ebde1f4699f498#code#L71

        IERC20(filledAssetToRebalance).transfer(
            destination,
            rebalAmt.sub(stratReward)
        );

Tools Used

none

Recommended Mitigation Steps

Add the OpenZepplin SafeERC20 library and replace all the transfer() and transferFrom() calls in your contracts with safeTransfer() and safeTransferFrom()

Lines of code

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

Vulnerability details

Impact

When using the RubiconRouter with ETH, the contract allows the user to transfer more ETH than is actually used within the transaction. The remaining ETH will simply be locked up in the Router contract causing the user to lose funds.

While this would be a mistake by the user, it's pretty easy to stop this from happening. So I think this is a valid issue.

Proof of Concept

In all the above linked code snippets, the contract verifies that msg.value >= x. Subsequent calls within the function will use x instead of msg.value. The user looses exactly x - msg.value ETH.

For example, the offerWithETH() function: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconRouter.sol#L391

    function offerWithETH(
        uint256 pay_amt, //maker (ask) sell how much
        // ERC20 nativeETH, //maker (ask) sell which token
        uint256 buy_amt, //maker (ask) buy how much
        ERC20 buy_gem, //maker (ask) buy which token
        uint256 pos //position to insert offer, 0 should be used if unknown
    ) external payable returns (uint256) {
        require(
            msg.value >= pay_amt,
            "didnt send enough native ETH for WETH offer"
        );
        uint256 _before = ERC20(buy_gem).balanceOf(address(this));
        WETH9(wethAddress).deposit{value: pay_amt}();
        uint256 id = RubiconMarket(RubiconMarketAddress).offer(
            pay_amt,
            ERC20(wethAddress),
            buy_amt,
            buy_gem,
            pos
        );
        uint256 _after = ERC20(buy_gem).balanceOf(address(this));
        if (_after > _before) {
            //return any potential fill amount on the offer
            ERC20(buy_gem).transfer(msg.sender, _after - _before);
        }
        return id;
    }

The user specifies the amount of tokens they want to offer in the pay_amt param. The function then checks whether msg.value is bigger or equal to pay_amt. After that, pay_amt of tokens are deposited into the WETH contract. Meaning, pay_amt - msg.value tokens are left unused in the Router contract.

Tools Used

none

Recommended Mitigation Steps

Verify that msg.value == x so that the user isn't allowed to pass more than necessary. That way you prevent the user from loosing any funds.

Lines of code

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

Vulnerability details

Impact

The BathBuddy contract is able to receive ETH. But, there's no way of ever retrieving that ETH from the contract. The funds will be locked up.

Currently, there seems to be no logic in the protocol where ETH is sent to the contract. But, it might happen in the future. So I'd say it's a MED issue.

Proof of Concept

receive() function: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/BathBuddy.sol#L69

Tools Used

none

Recommended Mitigation Steps

Remove the receive() function if the contract isn't supposed to handle ETH. Otherwise, add the necessary logic to release the ETH it gets.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L271 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/BathBuddy.sol#L87-L128

Vulnerability details

Impact

Bonus tokens are distributed with each withdrawal. The admin can add the same token multiple times which breaks the distribution. Users who withdraw early receive more than the rest.

Proof of Concept

setBonusToken() used to add bonus tokens: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L271

distributeBonusTokenRewards() is called when a user withdraws their funds: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L629

BathBuddy.release() handling the distribution: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/BathBuddy.sol#L87-L128

So if the same token is included twice in the bonusTokens array, the tokens are distributed twice for each withdrawal:

// we ignore fees here

bonusTokensReleasable: 30
userWithdrawal: 10
totalSupply: 30

1. release:
// see formula: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/peripheral_contracts/BathBuddy.sol#L103-L105
30 * 10 / 30 = 10

2. release within the same withdrawal since the token is included twice:
20  * 10 / 30 =  ~6.7

meaning, the user who withdrew receives ~16.7 of the 30 available bonus tokens

Tools Used

none

Recommended Mitigation Steps

Prohibit the same token to be added multiple times to the bonusTokens array in https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L271

Lines of code

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

Vulnerability details

Impact

The owner can set an arbitrary fee. If they set it to a value above 100%, withdrawing BathTokens won't be possible anymore because of an underflow. In RubiconMarket the owner can also have an arbitrary fee although that won't result in a DOS. But, the user might pay an absurdly high fee.

There should be checks that only allow fees up to a specific value, e.g. 10%.

Proof of Concept

For the DOS:

1. Owner sets fees to 101% using BathToken.setFeeBPS(): https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L261
2. User tries to withdraw which will revert here because fee > r: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L604

Tools Used

none

Recommended Mitigation Steps

Add a limit to the constructors where a fee is set and to all the configuration functions for fees.

Lines of code

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

Vulnerability details

Impact

The strategist is able to use user funds to trade on the RubiconMarket. They can abuse this to transfer user funds to themselves.

A strategist having access to user funds seems to be a deliberate design choice. But, I believe it's important to note how dangerous that is.

Proof of Concept

1. Strategist opens up an offer through placeMarketMakingTrades() where a token is sold for very cheap
2. Strategist accepts the offer within the same transaction using their private wallet

Tools Used

none

Recommended Mitigation Steps

There's no easy way to fix this since it's a big part of the protocol. You'd have to overhaul the whole thing.

You could minimize the dmg by limiting the amount of funds a strategist has access to

Lines of code

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

Vulnerability details

Impact

Through the BathHouse contract, an admin can allow people to use the BathPair contract to make trades with user funds. But, there's no way of removing people as strategists.

If a strategist turns rogue and abuses the power to make "bad" trades it will result in the loss of user funds. The admin has no way of stopping that.

Proof of Concept

Granting the strategist permission: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L265

Used within the following modifier to restrict access to BathPair functions: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathPair.sol#L149

Tools Used

none

Recommended Mitigation Steps

Update the function so that a boolean can be passed as param to be able to both approve and kick out strategists.

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L447 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L452-L462 https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L471

Vulnerability details

Impact

The ExpiringMarket is supposed to only allow buying and offering tokens at specific times. But the function that's supposed to handle that isn't properly implemented. Tokens can be bought and offered at all times.

The RubiconMarket contract inherits the ExpiringMarket so it's part of the core functionality. An important feature like that not working properly is a MED issue IMO.

Proof of Concept

Here's the short specification of the ExpiringMarket stating the purpose of the close time: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L447

Here's the modifier for the buy and offer function checking that the market isn't closed yet: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L452-L462

But, the isClosed() function always returns false: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L471

So the modifier's check will always pass allowing users to buy and offer at all times.

Tools Used

none

Recommended Mitigation Steps

Implement the isClosed() function properly.

Report

• Low
  • L-01: ExpiringMarket stopped state variable is unused
  • L-02: BathHouse.timeDelay state variable is unused
• Non-Critical
  • N-01: emit events when making changes to a contract's configuration

Low

L-01: ExpiringMarket stopped state variable is unused

The contract has a public state variable called stopped. It's used nowhere and has no deprecation notice. Consider removing it or if it's part of a planned feature, implement it properly.

Relevant code:

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

L-02: BathHouse.timeDelay state variable is unused

The contract has a public state varialbe called timeDelay. It's used nowhere. But, it's suppoed to be "A variable time delay after which a strategist must return funds to the Bath Token"

There's a function that's supposed to set the timeDelay of the BathToken but neither does the function do that nor does the BathToken contract support such a value: https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L286

Consider removing it or if it's part of a planned feature, implement it properly.

Non-Critical

N-01: emit events when making changes to a contract's configuration

Whenever you make changes to a contract's config it should emit an event. Especially when it's critical values like fees.

Here's a list of all the instances I found:

• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L1231
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/RubiconMarket.sol#L1256
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L245
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L250
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L260
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L265
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathToken.sol#L270
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L253
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L259
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L264
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L269
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L274
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L279
• https://github.com/code-423n4/2022-05-rubicon/blob/main/contracts/rubiconPools/BathHouse.sol#L336