Rubicon contest - pedroais's results

Lines of code

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

Vulnerability details

##Impact

Anyone can cancel orders from the router and get the tokens

##Proof of concept -A user makes a WETH order from the router

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

-Any attacker can call the cancel function with the order ID and get all the unfilled funds from the order

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

Basically orders in the router are up for grabs to the first who cancels then since the orders are made in name of the router, not the user msg.sender in cancelation will be the router.

Recommended Mitigation Steps

Only allow users to cancel orders they themselves sent

Lines of code

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

Vulnerability details

#impact Users could get exposed to higher risk than desired and funds to withdraw from the vault could not be available

Proof of Concept

The reserve ratio is the parameter that ensures a percentage of the tokens is always available to be withdrawn from a pool by the users. It also ensures a certain risk limit to how many funds can be risked at the same time. From the documentatión : enforceReserveRatio - this modifier ensures that the reserveRatio for each of the underlying liquidity pools (asset and quote bathTokens) is observed before and after function execution.

In bathPair.sol the reserve ratio is verified before but not after an offer is made so strategists can make trades that break the reserve ratio. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L344

Recommended Mitigation Steps

Enforce reserve ratio only after orders are placed. Doing it before doesn’t serve any purpose since if the reserve ratio isn’t met before but is met after a trade then that trade would actually be enforcing the ratio.

So the ratio should ideally be checked after execution and not checked before.

Lines of code

https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/6a0e1d89ff710f81428228d132e7d0c42de3a3cd/contracts/RubiconRouter.sol#L475

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

The withdrawForETH() function has a target pool address as input. This address is never validated. This means an attacker contract can be introduced as a fake pool.

The attacker contract could pass all the checks since all checks are calls to the contract.

Step by step : An attacker makes a malicious contract that implements the BathToken interface

When called targetPool.UnderlyingToken will return wethAddress to pass the check

The same can happen for targetPool.balanceOF and transferFrom

Finally targetPool.withdraw is called which doesn't withdraw anything since it's a fake pool but returns the router's ETH balance

Finally msg.sender.transfer(withdrawnWETH) happens and the attacker gets all the ETH from the contract

I consider this to be medium severity since it's not clear why the contract would hold ETH but the contract has a receive and fallback function so it's able to receive ETH.

Recommended Mitigation Steps

Add input validation for targetPool address

Lines of code

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/RubiconDeFi/rubicon-protocol-v1/blob/6a0e1d89ff710f81428228d132e7d0c42de3a3cd/contracts/rubiconPools/BathPair.sol#L601 https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/6a0e1d89ff710f81428228d132e7d0c42de3a3cd/contracts/rubiconPools/BathPair.sol#L615

Vulnerability details

ERC20 missing return value check

Impact

Funds may not be transferred but accounted as so

Proof of Concept

The erc20 interface ensures a token will return false on transfer failure but not necessarily revert. The return value should always be checked to account for these types of tokens.

This happens in various instances : 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/RubiconDeFi/rubicon-protocol-v1/blob/6a0e1d89ff710f81428228d132e7d0c42de3a3cd/contracts/rubiconPools/BathPair.sol#L601

https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/6a0e1d89ff710f81428228d132e7d0c42de3a3cd/contracts/rubiconPools/BathPair.sol#L615

Also all token transfers in rubiconRouter

Recommended Mitigation Steps

Use SafeERC20 library from openZeppelin

Lines of code

https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/6a0e1d89ff710f81428228d132e7d0c42de3a3cd/contracts/RubiconRouter.sol#L505

Vulnerability details

Impact

Users can lose funds

Proof of Concept

In the swapWithEth() function there is a pay amount input value is used to make the swap. msg.value is required to be greater than or equal to the pay amount plus the fee. If msg. value is greater then the extra ETH will be lost.

https://github.com/RubiconDeFi/rubicon-protocol-v1/blob/6a0e1d89ff710f81428228d132e7d0c42de3a3cd/contracts/RubiconRouter.sol#L505

Recommended Mitigation Steps

require msg.value == amtWithFee instead of >= or directly set pay amount equal to msg.value - fee

Lines of code

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

Vulnerability details

Impact

When a user withdraws from the vault the withdrawal fee is charged twice

Proof of Concept

The withdraw function computes shares from previewWithdraw() and then passes that share amount to _withdraw()

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

PreviewWithdraw() takes the amount subtracts the fee and returns the number of shares representing that amount

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

_withdraw then takes that share amount, converts it to asset again and subtract the fee

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

So the fee is being subtracted twice from the withdraw

Recommended Mitigation Steps

Instead of PreviewWithdraw() ConvertToShares() should be used to get the number of shares and charge the fee only once

Lines of code

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

Vulnerability details

Impact

Rugpull vector : Fee can be frontrunned to 100%. It can also be set to more than 100%. In that case substraction underflow and revert. Funds will be locked in the pool.

Proof of Concept

There is no limit to the fee which can be set to any percentage. This happens both in the market and in the withrawal fee for pools.

Recommended Mitigation Steps

Add a maximum fee

Lines of code

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

Vulnerability details

Any strategist can steal all funds from a given pool

Proof of Concept

The rebalancing mechanism ensures different tokens go to their corresponding pools after trades. Strategists can rebalance pools with tokens that aren’t their underlying asset to send tokens where they belong. They charge a fee for doing so.

A strategist can call the rebalance method with the same pool as asset and quote address. This means a pool will be rebalanced with itself so it will transfer funds to itself. It's a neutral action that doesn't change anything. The strategist will still get the rebalancing fee.

A strategist can rebalance a pool with itself an infinite amount of times and get the fee. In that way, he can drain all funds from any pool. This can only be done is a pool is rebalanced with itself wich shouldn’t happen.

Recommended Mitigation Steps

Check assetAddress != quote address

GAS G-01: Useless if statement. If the if is false the for loop wouldn’t start so the if can be safely deleted to save gas. https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L634

G-02: since askDelta = info.askPayAmt - offer1.pay_amt;

info.askPayAmt.sub(askDelta) will be equal to offer1.pay_amt

Making the substraction is not necesary and wastes gas. offer1.pay_amt can be used instead

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

Also

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