Rubicon contest - ilan's results

Lines of code

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

Vulnerability details

Impact

when calling transferFrom or transfer of an external token a bool indicating success / fail is returned. Not checking this return value can cause the transaction to proceed even when the transfer has failed.

for example in BathToken._withdraw token.transfer can fail for some reason and return false, but the withdrawal will succeed.

Proof Of Concept

From the ERC20 docs:

transfer(address recipient, uint256 amount) → bool external Moves amount tokens from the caller’s account to recipient.

Returns a boolean value indicating whether the operation succeeded.

Emits a Transfer event.

and same for transferFrom and approve.

An example for such asset is the Basic Attention Token which implements this transfer() and this transferFrom()

Tools Used

Manual Inspection with VSCode.

Recommended Mitigation Steps

check the return values of these calls: token.transferFrom(msg.sender, address(this), amount); -> require(token.transferFrom(msg.sender, address(this), amount), "transfer failed");

and the same for transfer()

or use the safe versions of these function which checks the return value

Lines of code

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

Vulnerability details

RubiconMarket does not support token with fee on transfer, can lock funds for some users

When creating a new offer with offer(), any ERC20 can be used for the pay_gem of the offer. If pay_gem is a token which takes a fee during the transfer, info.pay_amt will hold more than the amount which was actually transferred in pay_gem.transferFrom(msg.sender, address(this), pay_amt).

This can cause the contract to not have enough funds to transfer when users buy an offer, or when a user cancels his offer.

Proof Of Concept

In SimpleMarket.offer info.pay_amt is set to be the given amount in the parameter. However, there is no guarantee that the amount which is actually transferred is equal to that.

Example for such tokens is STA or PAXG. Example Implementation

A user can add an offer, and then call cancel which will fail because the contract does not have enough balance, and the user will lose his tokens.

Tools Used

Manual Inspection with VSCode.

Recommended Mitigation Steps

check the balance before and after the transfer, and set info.pay_amt to be the difference.

NC01 no need for == true

when checking if a boolean expression is satisfied, require(expression) is enough and there is no need for require(expression == true)

BathHouse.sol#L372 BathToken.sol#L228

NC02 It is good practice to document the parameters and return value in the function docs

The recommended format is:

/**
    * @dev general explanation
    *
    * @param param1 explanation
    * @param param2 explanation
    *
    * @return returnvalname explanation
    */

L01: Missing documentation

Many functions and complex logic are missing informative documentation and comments.

for example, RubicomMarket.getPayAmount is missing a general fucntion doc.

L02 BathToken is meant to be ERC-20 Token but does not implement IERC20

As written in the docs:

This contract represents a single-asset (underlyingToken) liquidity pool that users deposit into, in order to receive Bath Tokens (ERC-20 Token).

The bath token contract should is an ERC20. Even though it can still be used as an ERC20, it is better practice to have it implement the IERC20 interface or inherit from ERC20 (openzeppelin or as implemented in peripheral_contracts).

L03 BathPair.strategistBootyClaim is reenterable

BathPair.strategistBootyClaim

does not have a reentracy guard and is does not follow Checks Effects Interactions pattern. This can cause the strategist to reenter the code with certain tokens which call the msg.sender during the transfer, and claim all of the contracts amount of that token.

G01 should shorten long require error messages

Long messages take up a lot of gas. These messages should be short and informative. For example, in BathToken.withdraw

This long message:

"This implementation does not support non-sender owners from withdrawing user shares"

should be shortened, for example to

"Owner-only withdrawal"

G02 Cache multiple access to unchanged storage

Accesing a variable in storage takes al lot of gas, if the same variable is accessed multiple times but is not changed, it should be cached.

For example BathToken.sol#L635, in the loop:

for (uint256 index = 0; index < bonusTokens.length; index++) {

bonusTokens is a storage variable, and every access to it is expensive in gas. Changing the code to:

length = bonusTokens.length
for (uint256 index = 0; index < length; index++) {

will save a lot calls to the variable and lots of gas.

G03 Cache multiple access to unchanged memory

Accesing a variable in memory takes al lot of gas, if the same variable is accessed multiple times but is not changed, it should be cached.

For example, BathPair.sol#L582 in the loop:

for (uint256 index = 0; index < ids.length; index++) {

should be

length = ids.length
for (uint256 index = 0; index < length; index++) {

G04 Change postfix incrementation to prefix

postfix is generally more expensive because it must increment a value and return the old value, so it may require holding two numbers in memory. prefix only uses one number in memory.

Example:

for (uint256 index = 0; index < ids.length; index++) -> for (uint256 index = 0; index < ids.length; ++index) BathPair.sol#L582

G05 Use calldata

Use calldata instead of memory for external functions where the function argument is read-only. Reading directly from calldata using calldataload instead of going via memory saves the gas from the intermediate memory operations that carry the values.

G06 No need to explicitly initialize variables with default values

If a variable is not initialized it is automatically set to the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Example:

uint256 index = 0 -> uint256 index BathPair.sol#L582