code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/aave/AaveStrategy.sol#L209-L219 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/balancer/BalancerStrategy.sol#L120-L125 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/compound/CompoundStrategy.sol#L100-L108 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/convex/ConvexTricryptoStrategy.sol#L148-L156 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/curve/TricryptoNativeStrategy.sol#L182-L190 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L104-L112 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/stargate/StargateStrategy.sol#L193-L208 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/yearn/YearnStrategy.sol#L101-L106

Vulnerability details

Impact

The function emergencyWithdraw is to be sued by the owner in case of an emergency (exploit, risk management etc) to bring back all tokens to the strategy contract. However, there is nothing preventing the tokens to be sent back to their respective pools/vaults again! Thus any user can call deposit() with a single wei to force the contract to re-evaluate and send forth all the tokens back into the strategy which the owner has deemed unsafe.

The emergencyWithdraw are all implemented similarly, but here is a snippet from the Lido strategy.

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

        uint256 toWithdraw = stEth.balanceOf(address(this));
        uint256 minAmount = (toWithdraw * 50) / 10_000; //0.5%
        result = curveStEthPool.exchange(1, 0, toWithdraw, minAmount);

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

We can see that all the steth tokes will be swapped to WETH. But what happens when the next user comes and calls deposit again? This can be unintentional or a bad actor. The protocol immediately sends all the funds back into the unsafe protocol.

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
            emit AmountDeposited(queued);
            return;
        }

Here we see there are no checks. The function depositThreshold must be set to a very high value to prevent the strategy contract from sending out the tokens again.

Proof of Concept

The bug is obvious since there are no state changes in the emergencyWithdraw function.

Tools Used

Manual Review

Recommended Mitigation Steps

After an emergency withdraw, the depositThreshold must be set to a very high value to prevent the strategy contract from sending out the tokens again.

This can be done by adding this line to the very end of the emergencyWithdraw functions.

depositThreshold = type(uint256).max;

Assessed type

Other

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #1522

c4-judge commented 1 year ago

dmvt changed the severity to 2 (Med Risk)