Tapioca DAO - Ruhum's results

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L115 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L145 https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L130 https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L279

Vulnerability details

Impact

The Compound strategy uses the exchange rate calculated without accruing the interest first which can be smaller than the actual one. It depends on whether interest was already accrued in the given block. That will lead to the strategy to return a smaller number for its current balance.

The value is used by YieldBox to compute the number of shares to mint/burn when a user deposits/withdraws. For deposits that means the user gets more shares than they should while on withdrawals you receive fewer assets than you should.

Proof of Concept

The Compound strategy uses exchangeRateStored() to get the cToken's exchange rate:

    function _currentBalance() internal view override returns (uint256 amount) {
        uint256 shares = cToken.balanceOf(address(this));

        uint256 pricePerShare = cToken.exchangeRateStored();
        uint256 invested = (shares * pricePerShare) / (10 ** 18);
        uint256 queued = wrappedNative.balanceOf(address(this));
        return queued + invested;
    }

Instead, it should use exchangeRateCurrent() which accrues the interest for the pool before calculating the exchange rate: https://github.com/compound-finance/compound-protocol/blob/master/contracts/CToken.sol#L270C1-L286C6 also see docs

The strategy's balance is used in the deposit and withdrawal flow of YieldBox:

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

Tools Used

none

Recommended Mitigation Steps

Use exchangeRateCurrent() instead of exchangeRateStored().

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/tokens/TapOFT.sol#L197

Vulnerability details

Impact

If TapOFT is paused for more than two weeks, you won't be able to call emitForWeek(). That will cause

1. dso_supply to be inflated
2. unclaimed emissions not being rolled over to the next epoch

Proof of Concept

    ///-- Write methods --
    /// @notice Emit the TAP for the current week
    /// @return the emitted amount
    function emitForWeek() external notPaused returns (uint256) {
        require(_getChainId() == governanceChainIdentifier, "chain not valid");

        uint256 week = _timestampToWeek(block.timestamp);
        if (emissionForWeek[week] > 0) return 0;

        // Update DSO supply from last minted emissions
        dso_supply -= mintedInWeek[week - 1];
        // @audit if contract is paused for over a week, the minted tokens in a week
        // aren't subtracted from dso_supply. 

        // @audit if contract is paused for over a week, the remaining emissions
        // from the previous week won't be added to the current week's emissions.

        // Compute unclaimed emission from last week and add it to the current week emission
        uint256 unclaimed = emissionForWeek[week - 1] - mintedInWeek[week - 1];
        uint256 emission = uint256(_computeEmission());
        emission += unclaimed;
        emissionForWeek[week] = emission;

        emit Emitted(week, emission);

        return emission;
    }

Given that $100,000$ tokens were emitted in week $X$. Before the week ends the contract is paused. It's unpaused in week $X + 2$. emitForWeek() is executed:

• dso_supply -= mintedInWeek[week - 1];
  • because week == X+2, it will try to subtract mintedInWeek[x + 1] from dso_supply. But, no rewards were emitted in that week. The value will be 0. Thus, the funds emitted in week $X$ aren't subtracted from dso_supply.
• uint256 unclaimed = emissionForWeek[week - 1] - mintedInWeek[week - 1];
  • same thing, week - 1 == x + 1, where emissionForWeek[x + 1] == 0 and mintedInWeek[x + 1] == 0. Any unclaimed funds in week $X$ are not rolled over to the current epoch.

Tools Used

none

Recommended Mitigation Steps

The contract should keep track of the current week and that variable for the calculation:

    function emitForWeek() external notPaused returns (uint256) {
        require(_getChainId() == governanceChainIdentifier, "chain not valid");

        uint256 week = _timestampToWeek(block.timestamp);
        if (emissionForWeek[week] > 0) return 0;

        // Update DSO supply from last minted emissions
        dso_supply -= mintedInWeek[currentWeek];
        // @audit if contract is paused for over a week, the minted tokens in a week
        // aren't subtracted from dso_supply. 

        // @audit if contract is paused for over a week, the remaining emissions
        // from the previous week won't be added to the current week's emissions.

        // Compute unclaimed emission from last week and add it to the current week emission
        uint256 unclaimed = emissionForWeek[currentWeek] - mintedInWeek[currentWeek];
        uint256 emission = uint256(_computeEmission());
        emission += unclaimed;
        emissionForWeek[week] = emission;
        currentWeek = week;

        emit Emitted(week, emission);

        return emission;
    }

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L100 https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L155 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L123 https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/strategies/BaseStrategy.sol#L36

Vulnerability details

Impact

The owner of a YieldBox strategy can withdraw all of the deposited funds in case of an emergency. But, right after anybody can redeposit those funds back into the strategy because the admin isn't able to pause deposits.

We can assume that these funds are potentially at risk of being lost because the admin engaged in an emergency withdrawal. An attacker of the underlying protocol could bundle a tx where they redeposit the strategies funds into the underlying protocol to execute their attack.

Proof of Concept

Every YieldBox strategy has an emergencyWithdraw() function:

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

        uint256 toWithdraw = cToken.balanceOf(address(this));
        cToken.redeem(toWithdraw);
        INative(address(wrappedNative)).deposit{value: address(this).balance}();

        result = address(this).balance;
    }

All of the deposited funds are withdrawn from the underlying protocol and left in the strategy contract. Anybody is able to redeposit those funds by depositing 1 wei into YieldBox. That'll trigger the strategy's deposited() function which will deposit all available funds:

    function depositAsset(
        uint256 assetId,
        address from,
        address to,
        uint256 amount,
        uint256 share
    ) public allowed(from, assetId) returns (uint256 amountOut, uint256 shareOut) {
        // ...

        asset.strategy.deposited(amount);

        emit Deposited(msg.sender, from, to, assetId, amount, share, amountOut, shareOut, false);

        return (amount, share);
    }

    function _deposited(uint256 amount) internal override nonReentrant {
        uint256 queued = wrappedNative.balanceOf(address(this));
        if (queued > depositThreshold) {
            INative(address(wrappedNative)).withdraw(queued);

            cToken.mint{value: queued}();
            emit AmountDeposited(queued);
            return;
        }
        emit AmountQueued(amount);
    }

There's no way for the admin to remove the funds from the strategy contract. That's only possible by burning YieldBox shares through its withdraw() function. They also can't pause the YieldBox's deposit() function to prevent the user from re-deposting.

This issue affects all existing strategies.

Tools Used

none

Recommended Mitigation Steps

Make strategies pausable.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

tOLP tokens that are not unlocked after they have expired cause the reward distribution to be flawed. Rewards are distributed to expired locks and are not claimable.

Proof of Concept

In TapiocaOptionBroker.exerciseOption(), the caller's rewards are calculated through the total pool deposits and their share of it:

        uint256 eligibleTapAmount = muldiv(
            tOLPLockPosition.amount,
            gaugeTotalForEpoch,
            tOLP.getTotalPoolDeposited(tOLPLockPosition.sglAssetID)
        );

But, totalDeposited includes locks that have already expired. The user has to unlock their position for totalDeposited to be reduced:

    function getTotalPoolDeposited(
        uint256 _sglAssetId
    ) external view returns (uint256) {
        return
            activeSingularities[sglAssetIDToAddress[_sglAssetId]]
                .totalDeposited;
    }

    /// @notice Unlocks tOLP tokens
    /// @param _tokenId ID of the position to unlock
    /// @param _singularity Singularity market address
    /// @param _to Address to send the tokens to
    function unlock(
        uint256 _tokenId,
        IERC20 _singularity,
        address _to
    ) external returns (uint256 sharesOut) {
        require(_exists(_tokenId), "tOLP: Expired position");

        LockPosition memory lockPosition = lockPositions[_tokenId];
        require(
            block.timestamp >=
                lockPosition.lockTime + lockPosition.lockDuration,
            "tOLP: Lock not expired"
        );
        require(
            activeSingularities[_singularity].sglAssetID ==
                lockPosition.sglAssetID,
            "tOLP: Invalid singularity"
        );

        require(
            _isApprovedOrOwner(msg.sender, _tokenId),
            "tOLP: not owner nor approved"
        );

        _burn(_tokenId);
        delete lockPositions[_tokenId];

        // Transfer the tOLR tokens back to the owner
        sharesOut = yieldBox.toShare(
            lockPosition.sglAssetID,
            lockPosition.amount,
            false
        );

        yieldBox.transfer(
            address(this),
            _to,
            lockPosition.sglAssetID,
            sharesOut
        );
        // @audit totalDeposited is used for calculating the share of TAP rewards
        // of each user. If a user doesn't unlock their expired position, they are still assigned
        // rewards that they can't exercise.
        // Thus, they take away funds from actual participants.
        activeSingularities[_singularity].totalDeposited -= lockPosition.amount;

        emit Burn(_to, lockPosition.sglAssetID, lockPosition);
    }

Tools Used

none

Recommended Mitigation Steps

Currently, there's no way to force the user to unlock their position. There's no real incentive for them to unlock it immediately either. They can "park" the funds there.

If the user doesn't manually unlock their position after it has expired, it should be possible for third parties to do it for a small reward, e.g. 1% of the position.

That should incentivize users to unlock as early as possible.

Assessed type

Math

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/stargate/StargateStrategy.sol#L182 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/convex/ConvexTricryptoStrategy.sol#L207 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/convex/ConvexTricryptoStrategy.sol#L336 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L193

Vulnerability details

Impact

In multiple places, the minOut value for a swap is calculated inside the smart contract. That doesn't work. At the point where your calculation happens, the price has already been manipulated. You're not protecting yourself against sandwiches that way.

All of these swaps will probably be sandwiched causing a loss of funds for users.

Proof of Concept

Here's an example from the AAVE strategy:

            uint256 aaveAmount = aaveBalanceAfter - aaveBalanceBefore;

            //swap AAVE to wrappedNative
            ISwapper.SwapData memory swapData = swapper.buildSwapData(
                address(rewardToken),
                address(wrappedNative),
                aaveAmount,
                0,
                false,
                false
            );
            uint256 calcAmount = swapper.getOutputAmount(swapData, "");
            uint256 minAmount = calcAmount - (calcAmount * 50) / 10_000; //0.5%
            swapper.swap(swapData, minAmount, address(this), "");

Given that 1 ETH = 1,800 USDC:

• Attacker sandwiches your tx where the first tx moves the price to 1 ETH = 1,000 USDC
• your tx where you want to swap 1 ETH for USDC, your minOut calculation would result in:
  • calcAmount = 1,000 USDC because the price was manipulated.
  • minAmount = 1,000 USDC - 1,000 USDC * 50 / 10,000 = 995 USDC
  • meaning, as long as the swap results in you receiving 995 USDC it won't revert
• backrun your tx where they take the profit

By calculating the expected output amount on-chain, you're using already manipulated data. The minOut has to be calculated off-chain instead. Or, you use an oracle to get the real price and calculate minOut with that.

Tools Used

none

Recommended Mitigation Steps

Use an oracle to get the real price of an asset and calculate minOut using that.

Assessed type

MEV