QA Report

[code423n4]

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()