Rubicon contest - 0xDjango's results

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/RubiconRouter.sol#L194-L215

Vulnerability details

Impact

There are several ERC20 tokens that do not revert on failure. Therefore, it is possible for a function call containing transfer() or transferFrom() to succeed even if the ERC20 token transfer did not. This can lead to loss of funds for Rubicon users, specifically in the BathToken._withdraw() function. In the case of an ERC20 that does not revert on failure, if the contract does not have enough tokens to cover the transfer, the user will simply not receive any value from their withdrawal and their bath tokens will still be burnt.

Tools Used

Manual review

Recommended Mitigation Steps

Always use safeTransfer() and safeTransferFrom().

Lines of code

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

Vulnerability details

Impact

The migration function BathHouse.adminWriteBathToken() provides a rug vector for the admin of the protocol. They are able to receive deposits of underlying token and then switch the bath token contract associated with the underlying token to any contract they desire.

Tools Used

Manual review

Recommended Mitigation Steps

The presence of this function poses a security risk to the users of the protocol. Perhaps migration steps can be completed through a proposal process instead of at will by the owner of the protocol.

Lines of code

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

Vulnerability details

Impact

The owner is able to set the feeBPS as high as they like since no validation is performed in the setter. This can have two effects:

• By setting the feeBPS variable to 10000, the owner will effectively steal 100% percent of the withdraw value.
• By setting the feeBPS variable to 10001, the owner will DOS the withdrawal because the _withdraw() function will attempt to transfer more token to the feeTo than available for the withdrawal.

Tools Used

Manual review

Recommended Mitigation Steps

Apply some validation so that the fee can only be set within a specific range.

[L-01] Front-runnable initializer

The initialize() function lacks access control, allowing any malicious user to initialize the contract. This will require a redeploy of the contract if successful.

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

[L-02] Improper ownership transfer pattern

The BathToken.setBathHouse() function is used to change the bath house contract that is allowed to call the bath token contract's bath house-specific functions. The function lacks a proper ownership transfer pattern. It is recommended to make this a two-step process to ensure that the new bath house is truly the desired address. The first function called will set the pending contract and a second function must be called by the pending contract to accept the transfer.

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

[N-01] Unnecessary First Parentheses

Given the use of SafeMath, the first parentheses are unnecessary in this arithmetic.

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

[N-02] Duplicate import statements

The same contract is imported twice.

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

[G-01] The withdrawal function applies checks after transfer and event emissions

A lot of gas can be saved on failures by applying the following check prior to the token transfer and event emissions.

        require(
            assetsReceived >= assets,
            "You cannot withdraw the amount of assets you expected"
        );

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

[G-02] Use multiple requires instead of multiple &&

Using multiple require statements can be more performant than a string of &&'s.

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

[G-03] For loop optimization

Gas can be saved when performing for loops by following this pattern. The pattern moves the increment of i to an unchecked block removing the overflow checks, while also changing the increment to ++i.

for (uint i = 0; i < length;) {
    doStuff();
    unchecked {
        ++i;
    }
}

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

[G-04] Unnecessary initialization of variable

The initialization of variable newOne is unnecessary. Since the function returns newBathToken, it is not required to temporarily store this in newOne. Simply store it in newBathToken.

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