Tapioca DAO - Madalad's results

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/stargate/StargateStrategy.sol#L245-L246 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/stargate/StargateStrategy.sol#L216

Vulnerability details

Impact

In StargateStrategy, _currentBalance is used to determine the total collateral in terms of WETH. It is used in _withdraw to check whether the contract is solvent enough to process the withdrawal. However a miscalculation may lead to the returned value being less than the actual value, which can cause _withdraw to revert when it ought not to, preventing users from withdrawing and trapping funds in the contract. This would likely arise when a user attempts to withdraw all funds from the contract, for instance if they are the last user to withdraw from a strategy (e.g. if it is being deprecated).

Proof of Concept

_currentBalance fetches the amount of WETH in the contract (queued), the amount of LP tokens staked in the lpStaking contract (amount), and the amount of claimable rewards (claimableRewards), returning the sum of these values.

File: tapioca-yieldbox-strategies-audit\contracts\stargate\StargateStrategy.sol

214:     function _currentBalance() internal view override returns (uint256 amount) {
215:         uint256 queued = wrappedNative.balanceOf(address(this));
216:         (amount, ) = lpStaking.userInfo(lpStakingPid, address(this)); // @audit this returns amount of LP tokens, not WETH
217:         uint256 claimableRewards = compoundAmount();
218:         return amount + queued + claimableRewards;
219:     }

While queued and claimableRewards refer to amounts of WETH, amount refers to the amount of LP tokens staked, as is described in Stargate's LPStaking.sol contract. The amount of LP tokens staked is realistically never going to be equal to the amount of WETH used to acquire them, and so a discrepency exists.

Assume that _currentBalance returns a smaller value than the actual collateral amount of the contract (note: in a separate medium severity finding of mine titled "StargateStrategy#_withdraw: ether becomes trapped in the contract whenever a user withdraws", it is shown that 1 LP token < 1 WETH in value in the test environment of this repo).

When a user calls YieldBox#withdraw to withdraw funds from Stargate, StargateStrategy#_withdraw is executed and the check on line 246 may incorrectly cause a revert, preventing the user from accessing their funds. While this won't be an issue if there are many users utilising the strategy at the time, it will eventually become a problem for the unlucky user who is last to withdraw from the strategy.

File: tapioca-yieldbox-strategies-audit\contracts\stargate\StargateStrategy.sol

241:     function _withdraw(
242:         address to,
243:         uint256 amount
244:     ) internal override nonReentrant {
245:         uint256 available = _currentBalance();
246:         require(available >= amount, "StargateStrategy: amount not valid"); // @audit may incorrectly throw
247: 
248:         uint256 queued = wrappedNative.balanceOf(address(this));
249:         if (amount > queued) {
250:             compound("");
251:             uint256 toWithdraw = amount - queued;
252:             lpStaking.withdraw(lpStakingPid, toWithdraw);
253:             router.instantRedeemLocal(
254:                 uint16(lpRouterPid),
255:                 toWithdraw,
256:                 address(this)
257:             );
258: 
259:             INative(address(wrappedNative)).deposit{value: address(this).balance}();
260:         }
261: 
262:         require( // @audit redundant
263:             amount <= wrappedNative.balanceOf(address(this)),
264:             "Stargate: not enough"
265:         );
266:         wrappedNative.safeTransfer(to, amount);
267: 
268:         emit AmountWithdrawn(to, amount);
269:     }

Tools Used

Manual review

Recommended Mitigation Steps

In _currentBalance, calculate how much amount LP tokens are worth in WETH before adding it to the sum in the return statement.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Upon withdrawing funds from StargateStrategy, due to an incorrect assumption in _withdraw, the full amount is not refunded but instead remains in the contract as native ETH, resulting in a loss of funds for the withdrawing user.

Proof of Concept

We will assume here that amount > queued and the code inside the if statement is executed.

In _withdraw, the amount of LP tokens to withdraw is stored as toWithdraw, and withdrawn by calling lpStaking.withdraw (L252). Then, the Stargate Router is used to redeem these LP tokens and receive native ETH (L253-L257). The ether is wrapped (L259) and sent to the to address specified as a parameter (L266).

The problem is that the code assumes that the amount of ETH received after calling router.instantRedeemLocal is equal to the amount of LP tokens redeemed (toWithdraw), which is not necessarily the case (and indeed not likely). Since only toWithdraw amount of ETH is wrapped, the remainder is left in the contract after the transaction completes, resulting in a loss of funds for the user.

File: tapioca-yieldbox-strategies-audit\contracts\stargate\StargateStrategy.sol

241:     function _withdraw(
242:         address to,
243:         uint256 amount
244:     ) internal override nonReentrant {
245:         uint256 available = _currentBalance();
246:         require(available >= amount, "StargateStrategy: amount not valid");
247: 
248:         uint256 queued = wrappedNative.balanceOf(address(this));
249:         if (amount > queued) {
250:             compound("");
251:             uint256 toWithdraw = amount - queued;
252:             lpStaking.withdraw(lpStakingPid, toWithdraw);
253:             router.instantRedeemLocal(
254:                 uint16(lpRouterPid),
255:                 toWithdraw,
256:                 address(this)
257:             );
258: 
259:             INative(address(wrappedNative)).deposit{value: toWithdraw}(); // @audit incorrectly assuming toWithdraw = amount of ETH received
260:         }
261: 
262:         require(
263:             amount <= wrappedNative.balanceOf(address(this)),
264:             "Stargate: not enough"
265:         );
266:         wrappedNative.safeTransfer(to, amount);
267: 
268:         emit AmountWithdrawn(to, amount);
269:     }

To prove this, in stargateStrategy-fork.test.ts, alter the test titled "should allow deposits and withdrawals" to observe the ETH balance of the StargateStrategy contract before and after the indended execution flow has concluded:

    it('should allow deposits and withdrawals', async () => {
        const {
            stargateStrategy,
            weth,
            wethAssetId,
            yieldBox,
            deployer,
            binanceWallet,
            timeTravel,
        } = await loadFixture(registerFork);

+       console.log((await ethers.provider.getBalance(stargateStrategy.address)).toString());

        const routerEth = await stargateStrategy.addLiquidityRouter();
        const lpStakingContract = await ethers.getContractAt(
            'ILPStaking',
            await stargateStrategy.lpStaking(),
        );
        const lpStakingPid = await stargateStrategy.lpStakingPid();

        const poolInfo = await lpStakingContract.poolInfo(
            await stargateStrategy.lpStakingPid(),
        );
        const lpToken = await ethers.getContractAt(
            '@openzeppelin/contracts/token/ERC20/IERC20.sol:IERC20',
            poolInfo[0],
        );

        await yieldBox.registerAsset(
            1,
            weth.address,
            stargateStrategy.address,
            0,
        );

        const wethStrategyAssetId = await yieldBox.ids(
            1,
            weth.address,
            stargateStrategy.address,
            0,
        );
        expect(wethStrategyAssetId).to.not.eq(wethAssetId);
        const assetsCount = await yieldBox.assetCount();
        const assetInfo = await yieldBox.assets(assetsCount.sub(1));
        expect(assetInfo.tokenType).to.eq(1);
        expect(assetInfo.contractAddress.toLowerCase()).to.eq(
            weth.address.toLowerCase(),
        );
        expect(assetInfo.strategy.toLowerCase()).to.eq(
            stargateStrategy.address.toLowerCase(),
        );
        expect(assetInfo.tokenId).to.eq(0);

        const amount = ethers.BigNumber.from((1e18).toString()).mul(10);

        await weth
            .connect(binanceWallet)
            .transfer(deployer.address, amount.mul(10));

        await weth.approve(yieldBox.address, ethers.constants.MaxUint256);

        let share = await yieldBox.toShare(wethStrategyAssetId, amount, false);
        await yieldBox.depositAsset(
            wethStrategyAssetId,
            deployer.address,
            deployer.address,
            0,
            share,
        );

        let strategyWethBalance = await weth.balanceOf(
            stargateStrategy.address,
        );

        const lpStakingBalance = (
            await lpStakingContract.userInfo(
                lpStakingPid,
                stargateStrategy.address,
            )
        )[0];

        expect(strategyWethBalance.eq(0)).to.be.true;
        expect(lpStakingBalance.gt(0)).to.be.true;
        timeTravel(100 * 86400);
        share = await yieldBox.balanceOf(deployer.address, wethStrategyAssetId);
        await yieldBox.withdraw(
            wethStrategyAssetId,
            deployer.address,
            deployer.address,
            0,
            share,
        );
        strategyWethBalance = await weth.balanceOf(stargateStrategy.address);
        expect(strategyWethBalance.eq(0)).to.be.true;
        
+       console.log((await ethers.provider.getBalance(stargateStrategy.address)).toString());
    });

The output shows that indeed some ETH remains in the contract after the user deposits and withdraws the same amount:

  StargateStrategy fork test
0
816151744885982
    ✔ should allow deposits and withdrawals (5587ms)


  1 passing (7s)

Tools Used

Manual review, hardhat

Recommended Mitigation Steps

Change _withdraw to unwrap the correct amount of ETH:

    function _withdraw(
        address to,
        uint256 amount
    ) internal override nonReentrant {
        uint256 available = _currentBalance();
        require(available >= amount, "StargateStrategy: amount not valid");

        uint256 queued = wrappedNative.balanceOf(address(this));
        if (amount > queued) {
            compound("");
            uint256 toWithdraw = amount - queued;
            lpStaking.withdraw(lpStakingPid, toWithdraw);
            router.instantRedeemLocal(
                uint16(lpRouterPid),
                toWithdraw,
                address(this)
            );

-           INative(address(wrappedNative)).deposit{value: toWithdraw}();
+           INative(address(wrappedNative)).deposit{value: address(this).balance}();
        }

        require(
            amount <= wrappedNative.balanceOf(address(this)),
            "Stargate: not enough"
        );
        wrappedNative.safeTransfer(to, amount);

        emit AmountWithdrawn(to, amount);
    }

Assessed type

Math

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/MagnetarV2.sol#L214-L216 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/MagnetarV2.sol#L236-L238

Vulnerability details

Impact

A user who wishes to wrap the native token using MagnetarV2 will have their call always revert, unless they send double the amount of ether that is wrapped, and the excess ether simply remains in the Magnetar contract. This makes the expected functionality effectively useless.

Proof of Concept

The execution flow for calling MagnetarV2#burst with _action.id == TOFT_WRAP erroneously increments valAccumulator by _action.value twice: once before the large if statement that executes the call based on _action.id, and once inside the if block for _action.id == TOFT_WRAP. There is no need for lines 236-238 to exist.

File: tapioca-periph-audit\contracts\Magnetar\MagnetarV2.sol

213: 
214:             unchecked { // @audit call value is cached
215:                 valAccumulator += _action.value;
216:             }

                // ...

232:             } else if (_action.id == TOFT_WRAP) {
233:                 WrapData memory data = abi.decode(_action.call[4:], (WrapData));
234:                 _checkSender(data.from);
235:                 if (_action.value > 0) {
236:                     unchecked {
237:                         valAccumulator += _action.value; // @audit call value is cached again
238:                     }
239:                     ITapiocaOFT(_action.target).wrapNative{
240:                         value: _action.value
241:                     }(data.to);
242:                 } else {
243:                     ITapiocaOFT(_action.target).wrap(
244:                         msg.sender,
245:                         data.to,
246:                         data.amount
247:                     );
248:                 }

This means that the check at the end of the function, valAccumulator will be greater than msg.value by the amount that was wrapped, and the call will always revert. Note that if the user makes the call such that msg.value is twice the amount of _action.value of the wrapping action then the call will succeed, however the user will have sacrificed funds to the Magnetar contract.

File: tapioca-periph-audit\contracts\Magnetar\MagnetarV2.sol

714:         require(msg.value == valAccumulator, "MagnetarV2: value mismatch");

Tools Used

Manual review

Recommended Mitigation Steps

Remove the following lines:

File: tapioca-periph-audit\contracts\Magnetar\MagnetarV2.sol

            } else if (_action.id == TOFT_WRAP) {
                WrapData memory data = abi.decode(_action.call[4:], (WrapData));
                _checkSender(data.from);
                if (_action.value > 0) {
-                   unchecked {
-                       valAccumulator += _action.value;
-                   }
                    ITapiocaOFT(_action.target).wrapNative{
                        value: _action.value
                    }(data.to);
                } else {
                    ITapiocaOFT(_action.target).wrap(
                        msg.sender,
                        data.to,
                        data.amount
                    );
                }

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/MagnetarV2.sol#L732-L760 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/MagnetarV2.sol#L1064-L1075 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/modules/MagnetarMarketModule.sol#L702-L709

Vulnerability details

Impact

MagnetarV2#withdrawToChain allows users to withdraw assets from YieldBox to the destination chain of their choice. They must include a non-zero msg.value in order to pay for the gas that LayerZero requires to perform the operation. If the user specifies an asset that does not support cross chain operation, then the call will not revert, and the ether will not be refunded, leading to a loss of funds for the user.

Proof of Concept

Consider the following scenario: a user calls MagnetarV2#withdrawToChain to attempt a cross chain transfer of an asset that does not support cross chain operation.

File: tapioca-periph-audit\contracts\Magnetar\MagnetarV2.sol

732:     function withdrawToChain(
733:         IYieldBoxBase yieldBox,
734:         address from,
735:         uint256 assetId,
736:         uint16 dstChainId,
737:         bytes32 receiver,
738:         uint256 amount,
739:         uint256 share,
740:         bytes memory adapterParams,
741:         address payable refundAddress,
742:         uint256 gas
743:     ) external payable {
744:         _executeModule(
745:             Module.Market,
746:             abi.encodeWithSelector(
747:                 MagnetarMarketModule.withdrawToChain.selector,
748:                 yieldBox,
749:                 from,
750:                 assetId,
751:                 dstChainId,
752:                 receiver,
753:                 amount,
754:                 share,
755:                 adapterParams,
756:                 refundAddress,
757:                 gas
758:             )
759:         );
760:     }

_executeModule delegatecalls MagnetarMarketModule#withdrawToChain

File: tapioca-periph-audit\contracts\Magnetar\MagnetarV2.sol

1064:     function _executeModule(
1065:         Module _module,
1066:         bytes memory _data
1067:     ) private returns (bytes memory returnData) {
1068:         bool success = true;
1069:         address module = _extractModule(_module);
1070: 
1071:         (success, returnData) = module.delegatecall(_data);
1072:         if (!success) {
1073:             _getRevertMsg(returnData);
1074:         }
1075:     }

MagnetarMarketModule#_withdrawToChain is executed, and when the try catch fails, return is hit and the transaction completes.

File: tapioca-periph-audit\contracts\Magnetar\modules\MagnetarMarketModule.sol

702:         // make sure the asset supports a cross chain operation
703:         try
704:             IERC165(address(asset)).supportsInterface(
705:                 type(ISendFrom).interfaceId
706:             )
707:         {} catch {
708:             return;
709:         }

Since the user forwarded some ETH to pay for the LayerZero operation, and there is no refund funcionality for this path, the ether is trapped in the MagnetarV2 contract.

Tools Used

Manual review

Recommended Mitigation Steps

Make the call revert if a non-supporting asset is attempted to be sent cross chain:

File: tapioca-periph-audit\contracts\Magnetar\modules\MagnetarMarketModule.sol

702:         // make sure the asset supports a cross chain operation
703:         try
704:             IERC165(address(asset)).supportsInterface(
705:                 type(ISendFrom).interfaceId
706:             )
707:         {} catch {
- 708:          return;
+ 708:          revert("incompatible asset");
709:         }

Assessed type

ETH-Transfer

Lines of code

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

Vulnerability details

Impact

In the case of an emergency, the owner of any of the strategy contract implementations may call emergencyWithdraw to withdraw the entire balance from the external protocol back into the contract as wrapped native token. However a malicious user may deposit 1 wei straight after, which results in the entire withdrawn balance being re-deposited into the particular defi protocol, effectively undoing the emergency withdrawal immediately. It is also worth noting that any deposit, malicious or otherwise, that follows an emergency withdraw would have the same effect.

Proof of Concept

This issue is present in the following contracts from the tapioca-yieldbox-strategies-audit submodule:

• YearnStrategy.sol
• CompoundStrategy.sol
• LidoEthStrategy.sol
• TricryptoNativeStrategy.sol
• TricryptoLPStrategy.sol
• StargateStrategy.sol
• AaveStrategy.sol
• BalancerStrategy.sol
• ConvexTricryptoStrategy.sol

For brevity, we will focus on YearnStrategy here. When emergencyWithdraw is called, vault.withdraw is called to withdraw the strategy's entire balance, which results in the strategy contract receiving all of the deployed funds as wrappedNative tokens.

File: tapioca-yieldbox-strategies-audit\contracts\yearn\YearnStrategy.sol

101:     function emergencyWithdraw() external onlyOwner returns (uint256 result) {
102:         compound("");
103: 
104:         uint256 toWithdraw = vault.balanceOf(address(this)); // @audit withdraw all user funds
105:         result = vault.withdraw(toWithdraw, address(this), 0); // @audit this contract receives funds as wrapped native tokens
106:     }

At this point, the contract has a large balance of wrapped native tokens. Now, if a user wishes to deposit to the strategy, _deposited is executed.

File: tapioca-yieldbox-strategies-audit\contracts\yearn\YearnStrategy.sol

121:     function _deposited(uint256 amount) internal override nonReentrant {
122:         uint256 queued = wrappedNative.balanceOf(address(this));
123:         if (queued > depositThreshold) {
124:             vault.deposit(queued, address(this)); // @audit entire contract wrapped native balance is deposited
125:             emit AmountDeposited(queued);
126:             return;
127:         }
128:         emit AmountQueued(amount);
129:     }

This function checks the current amount of wrapped native tokens in the contract, and if it is above the depositThreshold, deposits all of them into the Yearn vault. Normally this is fine, as the users funds have already been transferred to this contract as wrapped native tokens earlier in the execution flow. However if an emergency withdrawal has just occurred, the sum of all user funds will also reside in the contract as wrapped native tokens. As such, all of these funds will be deposited into the Yearn vault (line 124).

By simply backrunning a call to emergencyWithdraw with a deposit of arbitrary amount, any user can immediately re-deposit all user funds back into the Yearn vault.

Tools Used

Manual review

Recommended Mitigation Steps

Add functionality to allow deposits to be paused when an emergency withdrawal has taken place. Such functionality may look something like this:

+   bool private depositsPaused;

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

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

+   function togglePause(bool _depositsPaused) external onlyOwner {
+       require(depositsPaused != _depositsPaused);
+       depositsPaused = _depositsPaused;
+   }

    // ...

    function _deposited(uint256 amount) internal override nonReentrant {
+       require(!depositsPaused, "deposits paused");
        uint256 queued = wrappedNative.balanceOf(address(this));
        if (queued > depositThreshold) {
            vault.deposit(queued, address(this));
            emit AmountDeposited(queued);
            return;
        }
        emit AmountQueued(amount);
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

YieldBox#depositETHAsset contains no checks on msg.value, and instead uses the user defined parameter amount to determine how much eth to wrap and deposit. This means that two things are possible:

1. users may erroneously send too much ether (msg.value > amount), in which case the ether remains in the contract after the transaction is executed
2. users may deliberately send not enough ether (msg.value < amount), in which case they would be spending any ether present in the contract before their call

Allowing users to send the incorrect amount of ether (1) and lose the excess is poor UX, and in this case the ether may be immediately stolen by other users (2).

Proof of Concept

The following test can be pasted in the "depositETH" section of YieldBox/test/YieldBox.ts. It shows that a user may overpay, and have their ether stolen by another user.

File: YieldBox\test\YieldBox.ts

217:     describe("depositETH", () => {
218:         it("YieldBox ether theft PoC", async function () {
219:             // Alice deposits 1000 but sends 1500
220:             await yieldBox.connect(alice).depositETH(ethStrategy.address, Alice, 1000, { value: 1500 })
221:             expect(await yieldBox.balanceOf(Alice, 2)).equals(1000_00000000)
222: 
223:             // Bob deposits 1000 but sends 500
224:             await yieldBox.connect(bob).depositETH(ethStrategy.address, Bob, 1000, { value: 500 })
225:             expect(await yieldBox.balanceOf(Bob, 2)).equals(1000_00000000)
226:         })

Tools Used

Manual review

Recommended Mitigation Steps

Ensure the correct msg.value is used when calling depositETHAsset:

File: YieldBox\contracts\YieldBox.sol

    function depositETHAsset(uint256 assetId, address to, uint256 amount) public payable returns (uint256 amountOut, uint256 shareOut) {
        // Checks
        Asset storage asset = assets[assetId];
        require(asset.tokenType == TokenType.ERC20 && asset.contractAddress == address(wrappedNative), "YieldBox: not wrappedNative");
+       require(msg.value == amount, "msg.value != amount");

        // Effects
        uint256 share = amount._toShares(totalSupply[assetId], _tokenBalanceOf(asset), false);

        _mint(to, assetId, share);

        // Interactions
        wrappedNative.deposit{ value: amount }();
        // Strategies always receive wrappedNative (supporting both wrapped and raw native tokens adds too much complexity)
        wrappedNative.safeTransfer(address(asset.strategy), amount);
        asset.strategy.deposited(amount);

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

        return (amount, share);
    }

Assessed type

ETH-Transfer

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/modules/MagnetarMarketModule.sol#L751 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/main/contracts/Magnetar/modules/MagnetarMarketModule.sol#L725-L731

Vulnerability details

Impact

In MagnetarMarketModule#_withdraw, the calculation for the gas parameter to pass to _withdrawToChain uses msg.value. However, if this call is made as part of a multicall using MagnetarV2#burst, then msg.value will include all ether to be spent on all of the calls combined, and as such it will be inflated. This can result in the function attempting to forward more ether than it can afford, which leads to a revert.

Proof of Concept

Consider a user who wishes to call burst to execute two operations (one of which involving a call to _withdraw), each of which has _action.value set to 1 ether. Then msg.value will equal 2 ether. When _withdraw is called, line 751 will set the gas variable to 2 ether, when it should be set to 1 ether. Since it will attempt to spend all of the calls ether (see line 725 in MagnetarMarketModule#_withdrawToChain), it will either revert because some ether has already been spent on a previous call, or it will spend the ether that should be reserved for future calls, leading to one of those future calls to revert.

File: tapioca-periph-audit\contracts\Magnetar\modules\MagnetarMarketModule.sol

734:     function _withdraw(
735:         address from,
736:         bytes memory withdrawData,
737:         IMarket market,
738:         IYieldBoxBase yieldBox,
739:         uint256 amount,
740:         uint256 share,
741:         bool withdrawCollateral
742:     ) private {
743:         require(withdrawData.length > 0, "MagnetarV2: withdrawData is empty");
744:         (
745:             bool withdrawOnOtherChain,
746:             uint16 destChain,
747:             bytes32 receiver,
748:             bytes memory adapterParams
749:         ) = abi.decode(withdrawData, (bool, uint16, bytes32, bytes));
750: 
751:         uint256 gas = msg.value > 0 ? msg.value : address(this).balance; // @audit msg.value is inflated as it contains eth to spend in other calls from burst()
752:         _withdrawToChain(
753:             yieldBox,
754:             from,
755:             withdrawCollateral ? market.collateralId() : market.assetId(),
756:             withdrawOnOtherChain ? destChain : 0,
757:             receiver,
758:             amount,
759:             share,
760:             adapterParams,
761:             gas > 0 ? payable(msg.sender) : payable(this),
762:             gas // @audit msg.value of entire burst() call
763:         );
764:     }

File: tapioca-periph-audit\contracts\Magnetar\modules\MagnetarMarketModule.sol

724:         // sends the asset to another layer
725:         ISendFrom(address(asset)).sendFrom{value: gas}( // @audit will fail as can't afford to send `gas` ETH
726:             address(this),
727:             dstChainId,
728:             receiver,
729:             amount,
730:             callParams
731:         );

Tools Used

Manual review

Recommended Mitigation Steps

Add a parameter to each function in MagnetarMarketModule that handles ether transfers, to be referenced instead of msg.value, so that it is possible to call multiple payable functions in a single call to burst.

Assessed type

DoS

Lines of code

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/lido/LidoEthStrategy.sol#L141-L167

Vulnerability details

Impact

In LidoEthStrategy#withdraw, the minAmount value which defines slippage tolerance is hardcoded to 2.5%. In the event that stETH depegs, and its value remains further than 2.5% below ETH, users will be unable to withdraw their funds from the strategy.

Note that emergencyWithdraw would also be DoS, however that is of lesser concern.

Proof of Concept

A stETH depeg is indeed possible, for example the stETH/ETH ratio dropped below 0.95 for an extended period during the LUNA collapse in 2022.

In such a case, when _withdraw is executed, the call to curveStEthPool.exchange will revert, as minAmount would be equal to 97.5% of the input stETH amount meanwhile price would be far below that. This means that any users attempt to withdraw would always revert.

File: tapioca-yieldbox-strategies-audit\contracts\lido\LidoEthStrategy.sol

141:     function _withdraw(
142:         address to,
143:         uint256 amount
144:     ) internal override nonReentrant {
145:         uint256 available = _currentBalance();
146:         require(available >= amount, "LidoStrategy: amount not valid");
147: 
148:         uint256 queued = wrappedNative.balanceOf(address(this));
149:         if (amount > queued) {
150:             uint256 toWithdraw = amount - queued; //1:1 between eth<>stEth
151:             uint256 minAmount = toWithdraw - (toWithdraw * 250) / 10_000; //2.5%  @audit hardcoded slippage control
152:             uint256 obtainedEth = curveStEthPool.exchange(
153:                 1,
154:                 0,
155:                 toWithdraw,
156:                 minAmount
157:             );
158: 
159:             INative(address(wrappedNative)).deposit{value: obtainedEth}();
160:         }
161:         queued = wrappedNative.balanceOf(address(this));
162:         require(queued >= amount, "LidoStrategy: not enough");
163: 
164:         wrappedNative.safeTransfer(to, amount);
165: 
166:         emit AmountWithdrawn(to, amount);
167:     }

The same issue is present in emergencyWithdraw:

File: tapioca-yieldbox-strategies-audit\contracts\lido\LidoEthStrategy.sol

104:     function emergencyWithdraw() external onlyOwner returns (uint256 result) {
105:         compound("");
106: 
107:         uint256 toWithdraw = stEth.balanceOf(address(this));
108:         uint256 minAmount = (toWithdraw * 50) / 10_000; //0.5%  @audit hardcoded slippage control
109:         result = curveStEthPool.exchange(1, 0, toWithdraw, minAmount);
110: 
111:         INative(address(wrappedNative)).deposit{value: result}();
112:     }

Tools Used

Manual review

Recommended Mitigation Steps

Ideally users would be able to define their own minAmount value, which would require changing the relevant functions in YieldBox and BaseStrategy. A simpler, albeit less optimal approach would be to allow the owner of the strategy to change the slippage control using a setter function. This way, in the case of a stETH depeg, the owner is able to react to allow users to withdraw their funds.

+   uint256 withdrawMinAmountBips = 250; //2.5%

+   function setWithdrawMinAmountBips(uint256 newWithdrawAmountBips) external onlyOwner {
+       require(newWithdrawAmountBips <= 10_000);
+       withdrawMinAmountBips = newWithdrawAmountBips;
+   }

    function _withdraw(
        address to,
        uint256 amount
    ) internal override nonReentrant {
        uint256 available = _currentBalance();
        require(available >= amount, "LidoStrategy: amount not valid");

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

            INative(address(wrappedNative)).deposit{value: obtainedEth}();
        }
        queued = wrappedNative.balanceOf(address(this));
        require(queued >= amount, "LidoStrategy: not enough");

        wrappedNative.safeTransfer(to, amount);

        emit AmountWithdrawn(to, amount);
    }

In the case of emergencyWithdraw, allowing the owner to specify minAmount as a parameter is simple and sufficient:

-   function emergencyWithdraw() external onlyOwner returns (uint256 result) {
+   function emergencyWithdraw(uint256 minAmount) 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}();
    }

Assessed type

DoS