Rubicon contest - rotcivegaf's results

Lines of code

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

Vulnerability details

Impact

Broke the EIP 2612

Proof of Concept

Some contract or dapp/backend could building the DOMAIN_SEPARATOR consulting the "rigth" name to the BathToken and build the signature with "rigth" digest message When these try to use the permit function (L713), with the "rigth" signature(v, r, s), the permit function will revert with the message "bathToken: INVALID_SIGNATURE" because the expect DOMAIN_SEPARATOR in the BathToken.sol contract was built with "wrong" name

Recommended Mitigation Steps

In the initialize function(L181) move the name definition before the DOMAIN_SEPARATOR definition

function initialize(
    ERC20 token,
    address market,
    address _feeTo
) external {
    require(!initialized);
    string memory _symbol = string(
        abi.encodePacked(("bath"), token.symbol())
    );
    symbol = _symbol;
    underlyingToken = token;
    RubiconMarketAddress = market;
    bathHouse = msg.sender; //NOTE: assumed admin is creator on BathHouse

    uint256 chainId;
    assembly {
        chainId := chainid()
    }
    name = string(abi.encodePacked(_symbol, (" v1")));
    DOMAIN_SEPARATOR = keccak256(
        abi.encode(
            keccak256(
                "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
            ),
            keccak256(bytes(name)),
            keccak256(bytes("1")),
            chainId,
            address(this)
        )
    );
    decimals = token.decimals(); // v1 Change - 4626 Adherence

    // Add infinite approval of Rubicon Market for this asset
    IERC20(address(token)).approve(RubiconMarketAddress, 2**256 - 1);
    emit LogInit(block.timestamp);

    feeTo = address(this); //This contract is the fee recipient, rewarding HODLers
    feeBPS = 3; //Fee set to 3 BPS initially

    // Complete constract instantiation
    initialized = true;
}

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

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

QA reports (low/non-critical)

Contest: Rubicon

Autor: Rotcivegaf

Scope:

Core:

• :white_check_mark: BathHouse.sol
• :white_check_mark: BathPair.sol
• :white_check_mark: BathToken.sol
• :white_check_mark: RubiconMarket.sol

Peripheral:

• :white_check_mark: RubiconRouter.sol
• :heavy_check_mark: BathBuddy.sol

:star: = Areas of concern :heavy_check_mark: = Audited Contract :white_check_mark: = Semi-Audited Contract(< 100%)

Non-critical

[N-01] Use standard libraries

Use @openzeppelin or @rari-capital/solmate to clarify code avoid mistake and don't repeat logic, PLEASE

[N-02] Missing error messages in require statements

• BathHouse.sol, lines: 88, 104, 110, 111, 280, 281
• BathPair.sol, lines: 118, 143
• BathToken.sol, lines: 186
• RubiconMarket.sol, lines: 210, 216, 217, 226, 379, 398-404, 416, 432, 453, 459, 460, 466, 467, 591, 595, 646, 687, 689, 702, 703, 715, 840, 865, 879, 906, 921, 945, 960, 970, 985, 1002, 1119, 1131, 1175, 1184, 1193, 1209
• RubiconRouter.sol, lines: 42

[N-03] Unused library

The library contract SafeMathE.sol its not used, remove it

[N-04] Move to test folder

Move the files IERC20.sol, ERC20.sol, TokenWithFaucet.sol to a test folder Also the you can use the IERC20.sol, ERC20.sol from @openzeppelin or @rari-capital/solmate and clean these

[N-05] Use IWETH interface

Import and use the interfaces/IWETH.sol in the RubiconRouter.sol instead of peripheral_contracts/WETH9.sol, solo move it to test folder

[N-06] Unused contract

The peripheral_contracts/VestingWallet.sol its imported from peripheral_contracts/BathBuddy.sol but only use the SafeMath library de @openzeppelin Also the interface interfaces/IVestingWallet.sol its unused

Import only the SafeMath library and remove VestingWallet.sol and IVestingWallet.sol contracts

[N-07] Redundant import

In BathBuddy.sol L6: The import its imported from IBathBuddy.sol

[N-08] Use type(uint256).max to clarify code

In BathToken.sol Lines 214, 256, 421, 451, 703: replace 2**256 - 1 and uint256(-1) to type(uint256).max

[N-09] Use the same parameter name

In BathToken.sol L134, L144: In event LogPoolCancel and LogPoolOffer use the same parameter name, e.g. orderId

[N-10] Unused function parameter

In BathToken.sol: Remove or comment function parameter name to mitigate this warnings

• L181, the _feeTo parameter:

function initialize(
    ERC20 token,
    address market,
    address /*_feeTo*/
) external {

• L416, the receiver parameter:

function maxDeposit(address /*receiver*/)
    public
    pure
    returns (uint256 maxAssets)
{

• L450, the receiver parameter:

function maxMint(address /*receiver*/) public pure returns (uint256 maxShares) {

[N-11] Return directly return the index, and revert if don't found it

In BathPair.sol, L311:

function getIndexFromElement(uint256 uid, uint256[] storage array)
    internal
    view
    returns (uint256 _index)
{
    for (uint256 index = 0; index < array.length; index++) {
        if (uid == array[index]) {
            return index;
        }
    }
    require("Didnt Find that element in live list, cannot scrub");
}

[N-12] Event is missing indexed fields

Each event should use three indexed fields if there are three or more fields

BathHouse.sol:

• L65-L71: LogNewBathToken
• L74-L82: LogOpenCreationSignal

BathPair.sol:

• L82-L90: LogStrategistTrade
• L93-L101: LogScrubbedStratTrade
• L104-L109: LogStrategistRewardClaim

BathToken.sol:

• L98-L106: LogDeposit
• L109-L119: LogWithdraw
• L122-L131: LogRebalance
• L134-L141: LogPoolCancel
• L144-L150: LogPoolOffer
• L153-L159: LogRemoveFilledTradeAmount

RubiconMarket.sol:

• L115: LogItemUpdate
• L180: OfferDeleted
• L513: LogMinSell
• L515: LogUnsortedOffer
• L516: LogSortedOffer
• L517: LogInsert
• L518: LogDelete
• L519: LogMatch

RubiconRouter.sol:

• L27-L35: LogSwap

BathBuddy.sol:

• L57-L64: LogClaimBonusToken

[N-13] Unused event

RubiconRouter.sol, L25: Remove the LogNote event

[N-14] Unsused interface

RubiconMarket.sol, L1262: Remove IWETH interface

[N-15] Remove comment code like contract ERC20 in RubiconMarket.sol, L94-L110

Low

[L-01] Add a upper bound

In BathToken.sol L256: The feeBPS, feeTo setter dont have a upper bound, by mistake could set high value and broke functions like _withdraw

uint256 public constant MAX_FEE_BPS = 10000; // 100 %
function setFeeBPS(uint256 _feeBPS) external onlyBathHouse {
    require(_feeBPS <= MAX_FEE_BPS, "Too high fee");
    feeBPS = _feeBPS;
}

uint256 public constant MAX_FEE_TO = 10000; // 100 %
function setFeeTo(address _feeTo) external onlyBathHouse {
    require(_feeTo <= MAX_FEE_TO, "Too high fee");
    feeTo = _feeTo;
}

[L-02] Add a != address(0) require in _mint function

In BathToken.sol L657:

function _mint(address to, uint256 value) internal {
    require(to != address(0), "ERC20: mint to the zero address");
    totalSupply = totalSupply.add(value);
    balanceOf[to] = balanceOf[to].add(value);
    emit Transfer(address(0), to, value);
}

[L-03] Add ability to remove tokens from bonusTokens array

In BathToken.sol: Add a remove bonus token function If add a wrong token by mistake there don't have a posibility to remove it

function removeBonusToken(uint256 index, address bonusERC20ToRemove) external onlyBathHouse {
    require(bonusTokens[index] == bonusERC20ToRemove, "Wrong index or bonusERC20ToRemove");

    bonusTokens[index] = bonusTokens[bonusTokens.length - 1];
    bonusTokens.pop();
}

PLEASE USE THIS AND IGNORE THE FIRST

Gas report

Contest: Rubicon

Autor: Rotcivegaf

Scope:

Core:

• :white_check_mark: BathHouse.sol
• :white_check_mark: BathPair.sol
• :white_check_mark: BathToken.sol
• :white_check_mark: RubiconMarket.sol

Peripheral:

• :white_check_mark: RubiconRouter.sol
• :heavy_check_mark: BathBuddy.sol

:star: = Areas of concern :heavy_check_mark: = Audited Contract :white_check_mark: = Semi-Audited Contract(< 100%)

Issues

[G-01] != 0 to > 0 for uint in require() statements

• RubiconMarket.sol, lines: 840, 879, 921, 945

Gas save: 6 for each require

* For the pragma versions minor than 0.8.13

[G-01] Repeated logged parameters

In BathToken.sol:

• L575-L584, The events LogDeposit and Deposit log repeated the assets, shares and msg.sender
• L607-L623, The events LogWithdraw and Withdraw log repeated the amountWithdrawn, _shares and msg.sender

Can remove from this parameters from LogDeposit and LogWithdraw events to save gas and clarify code

[G-02] Unnecessary type casting

In BathToken.sol L205: Remove the bytes cast

[G-03] Use function parameter

In BathToken.sol L214: Use market instead of RubiconMarketAddress to save the SLOAD

[G-04] <var>++ or <var> += 1 to ++<var>

• In BathPair.sol, lines 311, 427, 480, 582
• In BathToken.sol, line 635
• In RubiconRouter.sol, line 169, 227

Use ++index instead of index++ and unchecked the increment:

for (...; i++) {
    ...
}

to

unchecked {
for (...;) {
    ...
    unchecked {
        ++i;
    }
}

[G-05] Caching length on for

• In BathPair.sol, lines 311, 582
• In BathToken.sol, line 635
• In RubiconRouter.sol, line 169, 227

Caching the array length is more gas efficient:

for (uint256 i = 0; i < array.length; i++) {
  ...

to

uint256 arrayLength = array.length;

for (uint256 i = 0; i < arrayLength; i++) {
  ...

[G-06] Save the rewardsVestingWallet, feeBPS and bonusTokens.length in vars and directly bonusTokens[index] in release function call

In BathToken.sol, L629 distributeBonusTokenRewards function:

function distributeBonusTokenRewards(
    address receiver,
    uint256 sharesWithdrawn,
    uint256 initialTotalSupply
) internal {
    uint256 bonusTokensLength = bonusTokens.length;
    IBathBuddy _rewardsVestingWallet = rewardsVestingWallet;
    uint256 _feeBPS = feeBPS;

    if (bonusTokensLength > 0) {
        for (uint256 index = 0; index < bonusTokensLength; index++) {
            // Note: Shares already burned in Bath Token _withdraw

            // Pair each bonus token with a lightly adapted OZ Vesting wallet. Each time a user withdraws, they
            //  are released their relative share of this pool, of vested BathBuddy rewards
            // The BathBuddy pool should accrue ERC-20 rewards just like OZ VestingWallet and simply just release the withdrawer's relative share of releaseable() tokens
            if (_rewardsVestingWallet != IBathBuddy(0)) {
                _rewardsVestingWallet.release(
                    IERC20(bonusTokens[index]),
                    receiver,
                    sharesWithdrawn,
                    initialTotalSupply,
                    _feeBPS
                );
            }
        }
    }
}

[G-07] Save allowance[from][msg.sender] in var to save SLOAD gas cost

In BathToken.sol, L698 transferFrom function:

function transferFrom(
    address from,
    address to,
    uint256 value
) external returns (bool) {
    uint256 currentAllowance = allowance[from][msg.sender];
    if (currentAllowance != uint256(-1)) {
        allowance[from][msg.sender] = currentAllowance.sub(
            value
        );
    }
    _transfer(from, to, value);
    return true;
}

[G-08] Save IBathHouse(_bathHouse).getMarket() in a var

In BathToken.sol, L115: In initialize function save in a var and use msg.sender instead of _bathHouse to save SLOAD gas

function initialize(uint256 _maxOrderSizeBPS, int128 _shapeCoefNum)
    external
{
    require(!initialized);
    address _bathHouse = msg.sender; //Assume the initializer is BathHouse
    _rubiconMarket = IBathHouse(msg.sender).getMarket();

    require(
        _rubiconMarket !=
            address(0x0000000000000000000000000000000000000000) &&
            IBathHouse(msg.sender).initialized(),
        "BathHouse not initialized"
    );
    bathHouse = msg.sender;

    RubiconMarketAddress = _rubiconMarket;

...