Rubicon contest - WatchPug's results

Lines of code

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

Vulnerability details

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

function _deposit(uint256 assets, address receiver)
    internal
    returns (uint256 shares)
{
    uint256 _pool = underlyingBalance();
    uint256 _before = underlyingToken.balanceOf(address(this));

    // **Assume caller is depositor**
    underlyingToken.transferFrom(msg.sender, address(this), assets);
    uint256 _after = underlyingToken.balanceOf(address(this));
    assets = _after.sub(_before); // Additional check for deflationary tokens

    (totalSupply == 0) ? shares = assets : shares = (
        assets.mul(totalSupply)
    ).div(_pool);

    // Send shares to designated target
    _mint(receiver, shares);
    ...

A malicious early user can deposit()with1 weiofassettoken as the first depositor of theBathToken, and get 1 wei` of shares token.

This can be done by calling BathHouse.sol#openBathTokenSpawnAndSignal() with initialLiquidityNew: 1 wei:

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

Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) .

As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token.

They will immediately lose 9999e18 or half of their deposits if they redeem() right after the deposit.

Recommendation

Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a port of the initial mints as a reserve to the DAO so that the pricePerShare can be more resistant to manipulation.

function _deposit(uint256 assets, address receiver)
    internal
    returns (uint256 shares)
{
    uint256 _pool = underlyingBalance();
    uint256 _before = underlyingToken.balanceOf(address(this));

    // **Assume caller is depositor**
    underlyingToken.transferFrom(msg.sender, address(this), assets);
    uint256 _after = underlyingToken.balanceOf(address(this));
    assets = _after.sub(_before); // Additional check for deflationary tokens

    (totalSupply == 0) ? shares = assets : shares = (
        assets.mul(totalSupply)
    ).div(_pool);

    // for the first mint, we require the mint amount > (10 ** decimals) / 100
    // and send (10 ** decimals) / 1_000_000 of the initial supply as a reserve to DAO
    if (totalSupply == 0 && decimals >= 6) {
        require(shares > 10 ** (decimals - 2));
        uint256 reserveShares = 10 ** (decimals - 6);
        _mint(DAO, reserveShares);
        shares -= reserveShares;
    }

    // Send shares to designated target
    _mint(receiver, shares);
    ...

Lines of code

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

Vulnerability details

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

function underlyingBalance() public view returns (uint256) {
    uint256 _pool = IERC20(underlyingToken).balanceOf(address(this));
    return _pool.add(outstandingAmount);
}

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

function removeFilledTradeAmount(uint256 amt) external onlyPair {
    outstandingAmount = outstandingAmount.sub(amt);
    emit LogRemoveFilledTradeAmount(
        IERC20(underlyingToken),
        amt,
        underlyingBalance(),
        outstandingAmount,
        totalSupply
    );
}

For BathToken, there will be non-underlyingToken assets sitting on the contract that have filled to the contract and are awaiting rebalancing by strategists.

We assume the rebalance will happen periodically, between one rebalance to the next rebalance, underlyingBalance() will decrease over time as the orders get filled, so that the price per share will get lower while the actual equity remain relatively stable. This kind of price deviation will later be corrected by rebalancing.

Every time a BathPair.sol#rebalancePair() get called, there will be a surge of price per share for the BathToken, as a certain amount of underlyingToken will be transferred into the contract.

This enables a well known attack vector, which allows the pending yields to be stolen by front run the strategist's BathPair.sol#rebalancePair() transaction, deposit and take a large share of the vault, and withdraw() right after the rebalancePair() transaction for instant profit.

PoC

Given:

• Current underlyingBalance() is 100,000 USDC;
• Pending rebalancing amount is 1000 USDC;

1. strategist calls rebalancePair();
2. The attacker sends a deposit tx with a higher gas price to deposit 100,000 USDC, take 50% share of the pool;
3. After the transaction in step 1 is mined, the attacker calls withdraw() and retireve 100,500 USDC.

As a result, the attacker has stolen half of the pending yields in about 1 block of time.

Recommendation

Consider adding a new variable to track rebalancingAmount on BathToken.

BathToken should be notified for any pending rebalancing amount changes via BathPair in order to avoid sudden surge of pricePerShare over rebalancePair().

rebalancingAmount should be considered as part of underlyingBalance().

Lines of code

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

Vulnerability details

BathToken.sol#_deposit() calculates the actual transferred amount by comparing the before and after balance, however, since there is no reentrancy guard on this function, there is a risk of re-entrancy attack to mint more shares.

Some token standards, such as ERC777, allow a callback to the source of the funds (the from address) before the balances are updated in transferFrom(). This callback could be used to re-enter the function and inflate the amount.

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

function _deposit(uint256 assets, address receiver)
    internal
    returns (uint256 shares)
{
    uint256 _pool = underlyingBalance();
    uint256 _before = underlyingToken.balanceOf(address(this));

    // **Assume caller is depositor**
    underlyingToken.transferFrom(msg.sender, address(this), assets);
    uint256 _after = underlyingToken.balanceOf(address(this));
    assets = _after.sub(_before); // Additional check for deflationary tokens
    ...

PoC

With a ERC777 token by using the ERC777TokensSender tokensToSend hook to re-enter the deposit() function.

Given:

• underlyingBalance(): 100_000e18 XYZ.
• totalSupply: 1e18

The attacker can create a contracts with tokensToSend() function, then:

1. deposit(1) - preBalance = 100_000e18; - underlyingToken.transferFrom(msg.sender, address(this), 1)
2. reenter using tokensToSend hook for the 2nd call: deposit(1_000e18)
   • preBalance = 100_000e18;
   • underlyingToken.transferFrom(msg.sender, address(this), 1_000e18)
   • postBalance = 101_000e18;
   • assets (actualDepositAmount) = 101_000e18 - 100_000e18 = 1_000e18;
   • mint 1000 shares;
3. continue with the first deposit() call:
   • underlyingToken.transferFrom(msg.sender, address(this), 1)
   • postBalance = 101_000e18 + 1;
   • assets (actualDepositAmount) = (101_000e18 + 1) - 100_000e18 = 1_000e18 + 1;
   • mint 1000 shares;

As a result, with only 1 + 1_000e18 transferred to the contract, the attacker minted 2_000e18 XYZ worth of shares.

Recommendation

Consider adding nonReentrant modifier from OZ's ReentrancyGuard.

Lines of code

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

Vulnerability details

    if (to != address(this)) {
        ERC20(route[route.length - 1]).transfer(to, currentAmount);
    }

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

    underlyingToken.transfer(receiver, amountWithdrawn);

There some tokens do not revert on failure, but instead return false (e.g. ZRX).

It is usually good to add a require-statement that checks the return value or to use something like safeTransfer; unless one is sure the given token reverts in case of a failure.

Recommendation

Consider adding a require-statement or using safeTransfer.

Lines of code

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

Vulnerability details

There are ERC20 tokens that charge fee for every transfer() or transferFrom().

In the current implementation, RubiconMarket.sol#buy() and RubiconMarket.sol#offer() assumes that the received amount is the same as the transfer amount, and uses it to fulfill the offers.

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

offers[id].pay_amt = sub(_offer.pay_amt, quantity);
offers[id].buy_amt = sub(_offer.buy_amt, spend);
require(
        _offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend),
        "_offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend) failed - check that you can pay the fee"
);
require(
        _offer.pay_gem.transfer(msg.sender, quantity),
        "_offer.pay_gem.transfer(msg.sender, quantity) failed"
);

Recommendation

Consider comparing the before and after balanceOf to get the actual transferred amount.

Lines of code

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

Vulnerability details

function removeFilledTradeAmount(uint256 amt) external onlyPair {
    outstandingAmount = outstandingAmount.sub(amt);
    emit LogRemoveFilledTradeAmount(
        IERC20(underlyingToken),
        amt,
        underlyingBalance(),
        outstandingAmount,
        totalSupply
    );
}

In the current implementation, underlyingBalance() is IERC20(underlyingToken).balanceOf(address(this)) + outstandingAmount.

However, when an order is fulfilled or partially fulfilled, the buy_gem (non-underlyingToken assets) will send to this BathToken contract.

When the strategist calls BathPair.sol#scrubStrategistTrades() -> BathToken.sol#removeFilledTradeAmount(), the outstandingAmount will be reduced, thus suddenly decreases the price per share for this BathToken.

If a user redeems the shares now, they will suffer an unfair loss of value.

PoC

Given:

• BathTokenA = DAI
• BathTokenA.totalSupply = 10,000 * 1e18
• DAI balanceOf BathTokenA = 10,000 DAI

1. Alice deposited 10,000 DAI to BathTokenA Pool, got 10,000 * 1e18 shares

• DAI balanceOf BathTokenA = 20,000 DAI

2. Strategist called placeMarketMakingTrades on Pair, placed a offer: pay_amt = 1,000 * 1e18, buy_gem = USDC, buy_amt = 1,000 * 1e6

• DAI balanceOf BathTokenA = 19,000 DAI
• outstandingAmount = 1,000 * 1e18

3. Someone took the offer, sent 1,000 * 1e6 USDC to BathTokenA contract:

• DAI balanceOf BathTokenA = 19,000 DAI
• USDC balanceOf BathTokenA = 1,000 USDC
• outstandingAmount = 1,000 * 1e18

4. Strategist called scrubStrategistTrades() on Pair

• DAI balanceOf BathTokenA = 19,000 DAI
• USDC balanceOf BathTokenA = 1,000 USDC
• outstandingAmount = 0

5. Alice redeemed all shares, got 95,000 DAI back.

Recommendation

Consider adding a new variable to track rebalancingAmount on BathToken.

BathToken should be notified for any pending rebalancing amount changes via BathPair in order to avoid sudden surge of pricePerShare over rebalancePair().

rebalancingAmount should be considered as part of underlyingBalance().

Lines of code

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

Vulnerability details

    function isClosed() public pure returns (bool closed) {
        return false;
    }

After close, no new buys are allowed.

Based on context and comments, when the market is closed, offers can only be cancelled (offer and buy will throw).

However, in the current implementation, isClosed() always returns false, so the checks on whether the market is closed will always pass. (E.g: can_offer(), can_buy(), etc)

And there is a storage variable called stopped, but it's never been used, which seems should be used for isClosed.

Recommendation

Change to:

    function isClosed() public pure returns (bool closed) {
        return stopped;
    }

Lines of code

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

Vulnerability details

    uint256 r = (underlyingBalance().mul(_shares)).div(_initialTotalSupply);
    _burn(msg.sender, _shares);
    uint256 _fee = r.mul(feeBPS).div(10000);
    // If FeeTo == address(0) then the fee is effectively accrued by the pool
    if (feeTo != address(0)) {
        underlyingToken.transfer(feeTo, _fee);
    }
    amountWithdrawn = r.sub(_fee);
    underlyingToken.transfer(receiver, amountWithdrawn);

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

function underlyingBalance() public view returns (uint256) {
    uint256 _pool = IERC20(underlyingToken).balanceOf(address(this));
    return _pool.add(outstandingAmount);
}

In the current implementation, totalAssets is IERC20(underlyingToken).balanceOf(address(this)) + outstandingAmount.

However, when there are some outstandingAmount, the actual redeemable amount of underlyingToken is less than underlyingBalance().

As a result, when a user tries to redeem a larger amounts of shares, the transaction can revert due to insufficient balance.

PoC

Given:

• BathTokenA = USDT

1. Alice deposit 10,000 USDT to BathTokenA Pool, got 10,000 * 1e18 shares

• USDT balanceOf BathTokenA = 10,000 USDT

2. Strategist call placeMarketMakingTrades on Pair, place a offer: pay_amt = 1,000 * 1e18, buy_gem = USDC, buy_amt = 1,000 * 1e6

• USDT balanceOf BathTokenA = 9,000 USDT
• outstandingAmount = 1,000 * 1e18

3. Alice try redeem 9,500 * 1e18 shares, the transaction will revert.

Recommendation

Consider allowing the caller to trigger order cancelling when the balance in the contract is not enough for the withdrawal.

Furthermore, consider find a way to make the rebalancing permissionless so that the stake holders can always get back what they are owned without any centrialized roles.

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[M] Use short reason strings can save gas

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

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

        require(
            _offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend),
            "_offer.buy_gem.transferFrom(msg.sender, _offer.owner, spend) failed - check that you can pay the fee"
        );

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

        require(
            isClosed() || msg.sender == getOwner(id) || id == dustId,
            "Offer can not be cancelled because user is not owner, and market is open, and offer sells required amount of tokens."
        );

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

        require(
            _offer.pay_gem.transfer(msg.sender, quantity),
            "_offer.pay_gem.transfer(msg.sender, quantity) failed"
        );

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

        require(isActive(id), "Offer was deleted or taken, or never existed.");

[M] Redundant return for named returns

Redundant code increase contract size and gas usage at deployment.

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

    function isClosed() public pure returns (bool closed) {
        return false;
    }

Recommendation

    function isClosed() public pure returns (bool) {
        return false;
    }

[M] Setting uint256 variables to 0 is redundant

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

        uint256 old_top = 0;

Setting uint256 variables to 0 is redundant as they default to 0.

[S] Redundant check of uint128(pay_amt) == pay_amt

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

    function offer(
        uint256 pay_amt,
        ERC20 pay_gem,
        uint256 buy_amt,
        ERC20 buy_gem
    ) public virtual can_offer synchronized returns (uint256 id) {
        require(uint128(pay_amt) == pay_amt);
        require(uint128(buy_amt) == buy_amt);

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

    function buy(uint256 id, uint256 quantity)
        public
        virtual
        can_buy(id)
        synchronized
        returns (bool)
    {
        OfferInfo memory _offer = offers[id];
        uint256 spend = mul(quantity, _offer.buy_amt) / _offer.pay_amt;

        require(uint128(spend) == spend, "spend is not an int");
        require(uint128(quantity) == quantity, "quantity is not an int");

Recommendation

    function offer(
        uint128 pay_amt,
        ERC20 pay_gem,
        uint128 buy_amt,
        ERC20 buy_gem
    ) public virtual can_offer synchronized returns (uint256 id) {

    function buy(uint256 id, uint128 quantity)
        public
        virtual
        can_buy(id)
        synchronized
        returns (bool)
    {
        OfferInfo memory _offer = offers[id];
        uint256 spend = mul(quantity, _offer.buy_amt) / _offer.pay_amt;

        require(uint128(spend) == spend, "spend is not an int");