Rubicon contest - minhquanym's results

Lines of code

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

Vulnerability details

Impact

In BathToken._deposit() function, it uses underlyingToken balance of address(this) and totalSupply to calculate shares.

Attacker can transfer a large amount of underlyingToken into without calling deposit(), make the share’s price inflated.

Proof of concept

1. Attacker deposit 1 wei to mint 1 share

2. Attacker transfers huge amount to BathToken to greatly inflate the share’s price.

3. Subsequent depositors instead have to deposit an equivalent sum to avoid minting 0 shares. Otherwise, their deposits accrue to the attacker who holds the only share.

Tools Used

Manual Review

Recommened Mitigation Steps

Uniswap V2 solved this problem by sending the first 1000 LP tokens to the zero address. The same can be done in this case i.e. when totalSupply() == 0, send the first min liquidity LP tokens to the zero address to enable share dilution.

Ensure the number of shares to be minted is non-zero: require(_shares != 0, "zero shares minted");

Call deposit() once in initialize() to achieve the same effect as the suggestion above.

Judge has assessed an item in Issue #184 as Medium risk. The relevant finding follows:

Judge has assessed an item in Issue #184 as Medium risk. The relevant finding follows:

1. State stopped can be set to true but never use anywhere

Impact

In RubiconMarket there is a state stopped that can be set to true by using function stop()

But this stopped state is never used anywhere (functions, modifiers, …) else, so it’s useless

Proof of concept

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

Recommended Mitigation Steps

Remove state stopped

2. Named return variable but not use in ExpiringMarket.isClosed()

Impact

In ExpiringMarket.isClosed() function, return variable is bool closed but in the function, it’s just return false instead of assigning closed = false

Proof of concept

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

Recommended Mitigation Steps

Remove name closed for return variable or assigning closed = false inside function.

3. isOfferSorted() return true after call _unsort()

Impact

• In RubiconMarket._unsort() function, we should revmoe offer from the sorted list. The sorted list is the double linked list.
• But after _unsort, state prev and next of offer id is not deleted.
• Function isOfferSorted() check if next or prev not equal 0 then return true even the offer is _unsort()

Proof of concept

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

Recommended Mitigation Steps

Set prev and next of _rank[id] to 0 in _unsort function.

4. isClosed() is useless when always return false

Impact

Function isClosed() always return false so it’s useless.

Recommended Mitigation Steps

Remove function isClosed()

5. Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

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.

Occurences

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L353 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L357 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/BathToken.sol#L602 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/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

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently.

6. Old rubicon market still can use underlyingToken of BathToken

Impact

In BathToken, we add infinite approval of Rubicon Market for underlyingToken.

BathToken have a function setMarket() to set new Rubicon Market address and a function approveMarket() to add infinite approval

But contract don’t have any function to set approval to zero. So after set new Rubicon Market, old market still have infinite approval and still can use token of BathToken.

If a attacker has a way to manipulate Rubicon Market, even when team find out early and change to new version, he/she still can take all the funds in BathToken

Proof of concept

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L214 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L245-L247 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathToken.sol#L260-L262

Recommended Mitigation Steps

Set approval of old rubicon market to zero in setMarket()

1. Should validate input of external functions instead of internal functions.

Impact

In ERC20.transferFrom(), we use 2 internal functions _transfer() and _approve(), both check address != address(0). This is duplicated and not neccessary.

We are actually check != address(0) for sender 2 times and 1 time for msg.sender is not neccessary since msg.sender will always != address(0).

Similarly in approve(), increaseAllowance() and decreaseAllowance(), we check msg.sender != address(0) which is not necessary.

Proof of concepts

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/ERC20.sol#L269-L270 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/peripheral_contracts/ERC20.sol#L343-L344

Recommended Mitigations Steps

Validate input params in public functions instead of internal functions.

2. Use double linked-list instead of normal linked-list to save gas

Detail

In RubiconMarket, you uses normal linked-list for unsorted order list. And in _hide() you have to use a while loop through the whole list to find an offer with a given id which is inefficient.

Instead you can use a double linked-list for this, then find an offer with a given id and the previous, next ones will be in O(1).

Proof of concept

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

Recommended Mitigation Steps

Use double linked-list in unsorted order book, so remove 1 element from the list will be O(1)