Rubicon contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

The _deposit() function in BathToken calculates shares with rounding error and this error can be high when underlying token has (lower precisions and high price) or the ratio (underlyingBalance() / totalSupply() become high, by compounding or by attacker). so contract should return extra tokens (depositAmount - mintedShare * underlyingBalance() / totalSupply()) to depositor.

Proof of Concept

This _deposit() code in BathToken:

    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);
        emit LogDeposit(
            assets,
            underlyingToken,
            shares,
            msg.sender,
            underlyingBalance(),
            outstandingAmount,
            totalSupply
        );
        emit Deposit(msg.sender, msg.sender, assets, shares);
    }

As you can see in the line underlyingToken.transferFrom(msg.sender, address(this), assets); contract transfer deposit tokens and with this: shares = (assets.mul(totalSupply)).div(_pool) it calculates minted share amount. because of rounding in this line, user lose up to int( underlyingBalance() / totalSupply() ) amount of deposit amount (the minted share has lower value than deposited amount equal to rounding error). in some cases this lost can be very high, those are:

1. underlyingBalance() / totalSupply() become very high by compounding or by manipulation (transferring underlying token directly to contract address when contract balance is low)
2. when underlying token has low precision, for example 0 to 2.
3. when underlying token has high price, like more than $10K for each token. because these are common cases and attacker have some control over #1 so users will lose funds when depositing in some BathToken contracts.

Tools Used

VIM

Recommended Mitigation Steps

add more precision to shares or consider sending back extra amount of token deposited token by user.

Lines of code

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

Vulnerability details

Impact

Funds on BathHouse can be lost if token is deflationary or logics will not work.

Proof of Concept

This is openBathTokenSpawnAndSignal() in BathHouse code:

    function openBathTokenSpawnAndSignal(
        ERC20 newBathTokenUnderlying,
        uint256 initialLiquidityNew, // Must approve this contract to spend
        ERC20 desiredPairedAsset, // Must be paired with an existing quote for v1
        uint256 initialLiquidityExistingBathToken
    ) external returns (address newBathToken) {
        // Check that it doesn't already exist
        require(
            getBathTokenfromAsset(newBathTokenUnderlying) == address(0),
            "bathToken already exists for that ERC20"
        );
        require(
            getBathTokenfromAsset(desiredPairedAsset) != address(0),
            "bathToken does not exist for that desiredPairedAsset"
        );

        // Spawn a bathToken for the new asset
        address newOne = _createBathToken(newBathTokenUnderlying, address(0)); // NOTE: address(0) as feeAdmin means fee is paid to pool holders

        // Deposit initial liquidity posted of newBathTokenUnderlying
        require(
            newBathTokenUnderlying.transferFrom(
                msg.sender,
                address(this),
                initialLiquidityNew
            ),
            "Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol"
        );

        newBathTokenUnderlying.approve(newOne, initialLiquidityNew);

        // Deposit assets and send Bath Token shares to msg.sender
        IBathToken(newOne).deposit(initialLiquidityNew, msg.sender);

        // desiredPairedAsset must be pulled and deposited into bathToken
        require(
            desiredPairedAsset.transferFrom(
                msg.sender,
                address(this),
                initialLiquidityExistingBathToken
            ),
            "Couldn't transferFrom your initial liquidity - make sure to approve BathHouse.sol"
        );
        address pairedPool = getBathTokenfromAsset((desiredPairedAsset));
        desiredPairedAsset.approve(
            pairedPool,
            initialLiquidityExistingBathToken
        );

        // Deposit assets and send Bath Token shares to msg.sender
        IBathToken(pairedPool).deposit(
            initialLiquidityExistingBathToken,
            msg.sender
        );

        // emit an event describing the new pair, underlyings and bathTokens
        emit LogOpenCreationSignal(
            newBathTokenUnderlying,
            newOne,
            initialLiquidityNew,
            desiredPairedAsset,
            pairedPool,
            initialLiquidityExistingBathToken,
            msg.sender
        );

        newBathToken = newOne;
    }

As you can see it uses newBathTokenUnderlying.transferFrom(msg.sender,address(this),initialLiquidityNew) to transfer fund from user to contract and then uses IBathToken(newOne).deposit(initialLiquidityNew, msg.sender); to deposit them for msg.sender in newly created BathToken. but if token was deflationary, the amount that BathHouse receives from user can be lower than initialLiquidityNew, but BathHouse assumes it receives initialLiquidityNew tokens and deposits that amount in BathToken, so there is a chance that BathHouse receives less than initialLiquidityNew token from user and then transfer initialLiquidityNew to the BathToken which some of it is BathHouse's balance not user's. As you can see in _deposit() logic of BathToken it consider deflationary tokens:

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

Tools Used

VIM

Recommended Mitigation Steps

check for amount that really transferred to BathHouse

Lines of code

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

Vulnerability details

Impact

DOMAIN_SEPERATOR were calculated with wrong values (name value set after using it), so all the logics based on DOMAIN_SEPERATOR is not going to work properly.

Proof of Concept

This is where code calculates DOMAIN_SEPERATOR:

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

As you can see the value of name has been set after using it in DOMAIN_SEPARATOR calculation.

Tools Used

VIM

Recommended Mitigation Steps

set name value before using it

Lines of code

https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L492-L531 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L533-L563 https://github.com/code-423n4/2022-05-rubicon/blob/8c312a63a91193c6a192a9aab44ff980fbfd7741/contracts/rubiconPools/BathPair.sol#L210-L289

Vulnerability details

Impact

There is logical design bug in BathPair, and this bug can cause BathToken balances to be out of sync for some times and if in that time anyone withdraws or deposits to BathToken, amount will be calculated based on wrong values. The problem is when an order fills in market strategist needs to call handleStratOrderAtID() and (rebalancePair() or tailOff()) to update BathToken state. but contract don't make sure that strategist calls this two logic in one transaction, so if strategist do this in two transaction then for some time the underlyingBalance() for BathToken will be wrong and in that time any withdraw or deposit will be executed by wrong numbers.

Proof of Concept

Strategists place orders in market for BathTokens and BathToken keep track of these orders amount with outstandingAmount variable. when an orders fills in market strategist calls handleStratOrderAtID() in BathPair and it calls removeFilledTradeAmount() in BathPair to update outstandingAmount. This is handleStratOrderAtId() code which gets filled amount from market and calls removeFilledTradeAmount in BathToken to update outstandingAmount of BathToken.

    /// @notice This function results in the removal of the Strategist Trade (bid and/or ask on Rubicon Market) from the books and it being deleted from the contract
    /// @dev The local array of strategist IDs that exists for any given strategist [query via getOutstandingStrategistTrades()] acts as an acitve RAM for outstanding strategist trades
    /// @dev Cancels outstanding orders and manages the ledger of outstandingAmount() on bathTokens as Strategist Trades are cancelled/scrubbed or expired
    function handleStratOrderAtID(uint256 id) internal {
        StrategistTrade memory info = strategistTrades[id];
        address _asset = info.askAsset;
        address _quote = info.bidAsset;

        address bathAssetAddress = IBathHouse(bathHouse).tokenToBathToken(
            _asset
        );
        address bathQuoteAddress = IBathHouse(bathHouse).tokenToBathToken(
            _quote
        );
        order memory offer1 = getOfferInfo(info.askId); //ask
        order memory offer2 = getOfferInfo(info.bidId); //bid
        uint256 askDelta = info.askPayAmt - offer1.pay_amt;
        uint256 bidDelta = info.bidPayAmt - offer2.pay_amt;

        // if real
        if (info.askId != 0) {
            // if delta > 0 - delta is fill => handle any amount of fill here
            if (askDelta > 0) {
                logFill(askDelta, info.strategist, info.askAsset);
                IBathToken(bathAssetAddress).removeFilledTradeAmount(askDelta);
                // not a full fill
                if (askDelta != info.askPayAmt) {
                    IBathToken(bathAssetAddress).cancel(
                        info.askId,
                        info.askPayAmt.sub(askDelta)
                    );
                }
            }
            // otherwise didn't fill so cancel
            else {
                IBathToken(bathAssetAddress).cancel(info.askId, info.askPayAmt); // pas amount too
            }
        }

        // if real
        if (info.bidId != 0) {
            // if delta > 0 - delta is fill => handle any amount of fill here
            if (bidDelta > 0) {
                logFill(bidDelta, info.strategist, info.bidAsset);
                IBathToken(bathQuoteAddress).removeFilledTradeAmount(bidDelta);
                // not a full fill
                if (bidDelta != info.bidPayAmt) {
                    IBathToken(bathQuoteAddress).cancel(
                        info.bidId,
                        info.bidPayAmt.sub(bidDelta)
                    );
                }
            }
            // otherwise didn't fill so cancel
            else {
                IBathToken(bathQuoteAddress).cancel(info.bidId, info.bidPayAmt); // pass amount too
            }
        }

        // Delete the order from outOffersByStrategist
        uint256 target = getIndexFromElement(
            id,
            outOffersByStrategist[_asset][_quote][info.strategist]
        );
        uint256[] storage current = outOffersByStrategist[_asset][_quote][
            info.strategist
        ];
        current[target] = current[current.length - 1];
        current.pop(); // Assign the last value to the value we want to delete and pop, best way to do this in solc AFAIK

        emit LogScrubbedStratTrade(
            id,
            askDelta,
            _asset,
            bathAssetAddress,
            bidDelta,
            _quote,
            bathQuoteAddress
        );
    }

and this is removeFilledTradeAmount() code in BathToken which updates outstandingAmount by the data provided by BathToken.

    /// @notice A function called by BathPair to maintain proper accounting of outstandingAmount
    function removeFilledTradeAmount(uint256 amt) external onlyPair {
        outstandingAmount = outstandingAmount.sub(amt);
        emit LogRemoveFilledTradeAmount(
            IERC20(underlyingToken),
            amt,
            underlyingBalance(),
            outstandingAmount,
            totalSupply
        );
    }

So when an order fills in the market, as long as strategist don't call handleStratOrderAtID() the value of outstandingAmount will not show correct value for BathToken. And when an order fills in the market contract, BathToken will receive another token which strategist needs to call rebalancePair() or tailOff() in BathPair to rebalance BathTokens underlying token balances. So handleStratOrderAtID() updates outstandingAmount in BathToken and (rebalancePair() or tailOff()) updates BathTokens underlying token balance. and when orders fills in market contract strategist needs to call both of them to update BathToken's state. This is underlyingBalance() code in BathToken:

    /// @notice The best-guess total claim on assets the Bath Token has
    /// @dev returns the amount of underlying ERC20 tokens in this pool in addition to any tokens that are outstanding in the Rubicon order book seeking market-making yield (outstandingAmount)
    function underlyingBalance() public view returns (uint256) {
        uint256 _pool = IERC20(underlyingToken).balanceOf(address(this));
        return _pool.add(outstandingAmount);
    }

As you can see it uses underlyingToken balance plus outstandingAmount to calculate all the amouns in BathToken. This functions is used in deposit() and withdraw() of BathToken to calculate shares equivalent underlying amount or vice versa. when an orther fills in market contract strategist needs to call handleStratOrderAtID() and (rebalancePair() or tailOff()) to update underlyingToken balance and outstandingAmount in BathToken but there is no functionality in BathPair to do this two step in one transactions, so if strategist call them in two transaction, between this two transaction the state of BathToken will be wrong becasue underlyingBalance() will show wrong number and any withdraw() or deposit() in that time will be calculated wrong too. an attacker can exploit this by front-running or sandwich attack to steal others funds.

Tools Used

VIM

Recommended Mitigation Steps

make sure that state of BathToken gets updated in one step.

Lines of code

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

Vulnerability details

Impact

function rebalancePair() in BathPair is for rebalancing fills between two pools. but if strategist call it with _underlyingAsset==_underlyingQuote by mistake or intentionally then contract will make some dummy transfers and rebalance() in BathToken will charge some reward for strategist. so strategists can call this rapidly and create false reward for themself.

Proof of Concept

This is rebalancePair() code in BathPair:

/// @notice - function to rebalance fill between two pools
    function rebalancePair(
        uint256 assetRebalAmt, //amount of ASSET in the quote buffer
        uint256 quoteRebalAmt, //amount of QUOTE in the asset buffer
        address _underlyingAsset,
        address _underlyingQuote
    ) external onlyApprovedStrategist(msg.sender) {
        address _bathHouse = bathHouse;
        address _bathAssetAddress = IBathHouse(_bathHouse).tokenToBathToken(
            _underlyingAsset
        );
        address _bathQuoteAddress = IBathHouse(_bathHouse).tokenToBathToken(
            _underlyingQuote
        );
        require(
            _bathAssetAddress != address(0) && _bathQuoteAddress != address(0),
            "tokenToBathToken error"
        );

        // This should be localized to the bathToken in future versions
        uint16 stratReward = IBathHouse(_bathHouse).getBPSToStrats();

        // Simply rebalance given amounts
        if (assetRebalAmt > 0) {
            IBathToken(_bathQuoteAddress).rebalance(
                _bathAssetAddress,
                _underlyingAsset,
                stratReward,
                assetRebalAmt
            );
        }
        if (quoteRebalAmt > 0) {
            IBathToken(_bathAssetAddress).rebalance(
                _bathQuoteAddress,
                _underlyingQuote,
                stratReward,
                quoteRebalAmt
            );
        }
    }

As you can see it transfers funds (the filled orders funds) between two registered BathToken by calling rebalance() in them. This is rebalance() code in BathToken:

    function rebalance(
        address destination,
        address filledAssetToRebalance, /* sister or fill asset */
        uint256 stratProportion,
        uint256 rebalAmt
    ) external onlyPair {
        uint256 stratReward = (stratProportion.mul(rebalAmt)).div(10000);
        IERC20(filledAssetToRebalance).transfer(
            destination,
            rebalAmt.sub(stratReward)
        );
        IERC20(filledAssetToRebalance).transfer(msg.sender, stratReward);

        emit LogRebalance(
            IERC20(underlyingToken),
            destination,
            IERC20(filledAssetToRebalance),
            rebalAmt,
            stratReward,
            underlyingBalance(),
            outstandingAmount,
            totalSupply
        );
    }

As you can see it sends funds to defined destination and send some rewards to BathPair to be distributed to strategists later. But a malicious strategist or just by mistake can call rebalancePair() with _underlyingAsset==_underlyingQuote and then BathToken of that asset would make some dummy transfer from pool to himself but send the rewards to BathPair which strategist didn't earn it. (based on token implementation this can be fail) calling rebalancePair() by strategists with this values again and again can drain BathToken balances.

Tools Used

VIM

Recommended Mitigation Steps

in rebalancePair() check that _underlyingAsset != _underlyingQuote.

Lines of code

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

Vulnerability details

Impact

Function tailOff() is callable by strategist and it sends BathToken funds to _stratUtil address. There is no whitelist check for the destination address and if strategist by mistake or intentionally call tailOff() by wrong values liquidity providers funds will be lost.

Proof of Concept

This is tailOff() code in BathPair:

    /// @notice Function to attempt inventory risk tail off on an AMM
    /// @dev This function calls the strategist utility which handles the trade and returns funds to LPs
    function tailOff(
        address targetPool,
        address tokenToHandle,
        address targetToken,
        address _stratUtil, // delegatecall target
        uint256 amount, //fill amount to handle
        uint256 hurdle, //must clear this on tail off
        uint24 _poolFee
    ) external onlyApprovedStrategist(msg.sender) {
        // transfer here
        uint16 stratRewardBPS = IBathHouse(bathHouse).getBPSToStrats();

        IBathToken(targetPool).rebalance(
            _stratUtil,
            tokenToHandle,
            stratRewardBPS,
            amount
        );

        // Should always exceed hurdle given amountOutMinimum
        IStrategistUtility(_stratUtil).UNIdump(
            amount.sub((stratRewardBPS.mul(amount)).div(10000)),
            tokenToHandle,
            targetToken,
            hurdle,
            _poolFee,
            targetPool
        );
    }

As you can see it calls rebalance() of targetPool and it will send amount underlying token to _stratUtil address. if strategist set _stratUtil value wrongly then users funds will be lost forever. There should be some whitelist for _stratUtil that strategist can only send funds to them, is not secure to totally deponed on strategists not making mistake or not doing anything wrong.

Tools Used

VIM

Recommended Mitigation Steps

add _stratUtil addresses list in BathPair and confirm them.

Lines of code

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

Vulnerability details

Impact

Attacker can cause DOS and griefing and in some scenarios steal user funds. Deployer needs to call startErUp() after he deployed RubiconRouter but attacker can watch blockchain transactions and call startErUp() immediately after any RubiconRouter contract deployment to set RubiconMarketAddress and wethAddress and steal funds anybody sends to contract also deployer will lose deployment payed gas and need to deploy another version of RubiconRouter but attacker can perform attack for all the deployments.

Proof of Concept

This is startErUp() code in RubiconRouter contract:

    function startErUp(address _theTrap, address payable _weth) external {
        require(!started);
        RubiconMarketAddress = _theTrap;
        wethAddress = _weth;
        started = true;
    }

As you can see there is no access control and it is only callable one time and it sets important parameters of contract. attacker can call this function by watching new transactions on blockchain nodes and call it with sandwich attack to perform DOS or griefing or steal user funds.

Tools Used

VIM

Recommended Mitigation Steps

add constructor for RubiconRouter and authorization for startErUp()