Tapioca DAO - ladboy233's results

Lines of code

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

Vulnerability details

Impact

malicious user can drain the allowance of "from" account using addCollateral in BigBang.sol

Proof of Concept

In BigBang.sol borrowing function call

function addCollateral(
	address from,
	address to,
	bool skim,
	uint256 amount,
	uint256 share
) public allowedBorrow(from, share) notPaused {
	_addCollateral(from, to, skim, amount, share);
}

note the modifier

allowedBorrow(from, share)

this is calling allowBorrow

    function _allowedBorrow(address from, uint share) internal {
        if (from != msg.sender) {
            if (allowanceBorrow[from][msg.sender] < share) {
                revert NotApproved(from, msg.sender);
            }
            allowanceBorrow[from][msg.sender] -= share;
        }
    }

but this modifier check can be bypassed by setting share to 0 and specify an "amount" amount

   function addCollateral(
        address from,
        address to,
        bool skim, // we set this to false
        uint256 amount, // we specify an amount
        uint256 share // we set this to 0 to bypass the modifer check
    ) public allowedBorrow(from, share) notPaused {

then we are calling add collateral

  function _addCollateral(
        address from,
        address to,
        bool skim,
        uint256 amount,
        uint256 share
    ) internal {
        if (share == 0) {
            share = yieldBox.toShare(collateralId, amount, false);
        }
        userCollateralShare[to] += share;
        uint256 oldTotalCollateralShare = totalCollateralShare;
        totalCollateralShare = oldTotalCollateralShare + share;
        _addTokens(from, collateralId, share, oldTotalCollateralShare, skim);
        emit LogAddCollateral(skim ? address(yieldBox) : from, to, share);
    }

note:

if (share == 0) {
	share = yieldBox.toShare(collateralId, amount, false);
}
userCollateralShare[to] += share;

by setting share to 0 but inputting amount, the code still calculate the share for us and the collateral share goes to account "to", a malicious user's account

and then function _addToken force address "from" to pay as long as the address "from" has unused allowance

function _addTokens(
	address from,
	uint256 _tokenId,
	uint256 share,
	uint256 total,
	bool skim
) internal {
	if (skim) {
		require(
			share <= yieldBox.balanceOf(address(this), _tokenId) - total,
			"BigBang: too much"
		);
	} else {
		yieldBox.transfer(from, address(this), _tokenId, share);
	}
}

the address "from" loss fund in this line of code:

yieldBox.transfer(from, address(this), _tokenId, share);

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol validate the _allowBorrow after calcuting the share amout using the "amount" parameter if the share is set to 0

    function _addCollateral(
        address from,
        address to,
        bool skim,
        uint256 amount,
        uint256 share
    ) internal {
        if (share == 0) {
            share = yieldBox.toShare(collateralId, amount, false);
@@@         _allowBorrow(from, share) // new check added
        }
        userCollateralShare[to] += share;
        uint256 oldTotalCollateralShare = totalCollateralShare;
        totalCollateralShare = oldTotalCollateralShare + share;
        _addTokens(from, collateralId, share, oldTotalCollateralShare, skim);
        emit LogAddCollateral(skim ? address(yieldBox) : from, to, share);
    }

and we also recommend does not use the address "from" to check the allowance, use msg.sender instead

Assessed type

Invalid Validation

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L120 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/Market.sol#L414 https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L447 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/bigBang/BigBang.sol#L758

Vulnerability details

Impact

LidoEthStrategy in YieldBox is not safe to use because it is using the spot price to calculate the balance, then user can inflate the share to borrow arbitrary amount asset or deny liquidation

Proof of Concept

The issue is in LidoEthStrategy, the code use the spot price in curve pool to calculate the current balance of the yield stragety

   /// @dev queries Lido and Curve Eth/STEth pools
    function _currentBalance() internal view override returns (uint256 amount) {
        uint256 stEthBalance = stEth.balanceOf(address(this));
        // @audit
        // this is spot price, vulnerable to manipulation
        uint256 calcEth = stEthBalance > 0
            ? curveStEthPool.get_dy(1, 0, stEthBalance)
            : 0;

        uint256 queued = wrappedNative.balanceOf(address(this));
        return calcEth + queued;
    }

why this is a issue?

The explanation is easier if we take a look at the BigBang.sol code and take a top-down approach to walk through the code

First

user can add collateral in BigBang.sol

add collateral means transferring yield box share to the contract

We are calling _addCollateral

and the share collateral is added

userCollateralShare[to] += share;

then we call _addToken to transfer the share in

 yieldBox.transfer(from, address(this), _tokenId, share);

now we add the collateral share

second

User can use this collateral share as collateral to borrow asset

when calling borrow

we are calling

uint256 allowanceShare = _computeAllowanceAmountInAsset(
		from,
		exchangeRate,
		amount,
		asset.safeDecimals()
	);
	_allowedBorrow(from, allowanceShare);
	(part, share) = _borrow(from, to, amount);

calling _computeAllowanceAmountInAsset

    function _computeAllowanceAmountInAsset(
        address user,
        uint256 _exchangeRate,
        uint256 borrowAmount,
        uint256 assetDecimals
    ) internal view returns (uint256) {
        uint256 maxBorrowabe = _computeMaxBorrowableAmount(user, _exchangeRate);

        uint256 shareRatio = _getRatio(
            borrowAmount,
            maxBorrowabe,
            assetDecimals
        );
        return (shareRatio * userCollateralShare[user]) / (10 ** assetDecimals);
    }

calling _computeMaxBorrowableAmount

    function _computeMaxBorrowableAmount(
        address user,
        uint256 _exchangeRate
    ) internal view returns (uint256 collateralAmountInAsset) {
        collateralAmountInAsset =
            yieldBox.toAmount(
                collateralId,
                (userCollateralShare[user] *
                    (EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
                    collateralizationRate),
                false
            ) /
            _exchangeRate;
    }

this is important, the collateralAmountInAsset worth is calculated using how many share the user deposit and then we are calling yieldBox.toAmount

this is calling yieldBox.toAmount

   function toAmount(uint256 assetId, uint256 share, bool roundUp) external view returns (uint256 amount) {
        if (assets[assetId].tokenType == TokenType.Native || assets[assetId].tokenType == TokenType.ERC721) {
            amount = share;
        } else {
            amount = share._toAmount(totalSupply[assetId], _tokenBalanceOf(assets[assetId]), roundUp);
        }
    }

what is _tokenBalanceOf?

it is calling this code

function _tokenBalanceOf(Asset storage asset) internal view returns (uint256 amount) {
	return asset.strategy.currentBalance();
}

and if the stragety is LidoEthStrategy, we are calling

   /// @dev queries Lido and Curve Eth/STEth pools
    function _currentBalance() internal view override returns (uint256 amount) {
        uint256 stEthBalance = stEth.balanceOf(address(this));
        // @audit
        // this is spot price, vulnerable to manipulation
        uint256 calcEth = stEthBalance > 0
            ? curveStEthPool.get_dy(1, 0, stEthBalance)
            : 0;

        uint256 queued = wrappedNative.balanceOf(address(this));
        return calcEth + queued;
    }

a malicious user can use flashloan to manipulate the curveStEthPool liquidity to inflate the share worth and then borrow large amount of asset (minting large amount of USDO)

//mint USDO
IUSDOBase(address(asset)).mint(address(this), amount);

//deposit borrowed amount to user
asset.approve(address(yieldBox), amount);
yieldBox.depositAsset(assetId, address(this), to, amount, 0);

In the event when the underlying collateral share worth drops, a user's borrowed position may subject to liquidation

but before the liqudation,

the function is called to make sure that if user's position is solvent, do not perform liquidation

if (!_isSolvent(user, _exchangeRate)) {
	liquidatedCount++;
	_liquidateUser(
		user,
		maxBorrowParts[i],
		swapper,
		_exchangeRate,
		swapData
	);
}

In this [_isSolvent check], the code tries to convert the share amount to share worth in asset then multiply by the exchange rate to determine the collateral worth

    function _isSolvent(
        address user,
        uint256 _exchangeRate
    ) internal view returns (bool) {
        // accrue must have already been called!
        uint256 borrowPart = userBorrowPart[user];
        if (borrowPart == 0) return true;
        uint256 collateralShare = userCollateralShare[user];
        if (collateralShare == 0) return false;

        Rebase memory _totalBorrow = totalBorrow;

        return
            yieldBox.toAmount(
                collateralId,
                collateralShare *
                    (EXCHANGE_RATE_PRECISION / FEE_PRECISION) *
                    collateralizationRate,
                false
            ) >=
            // Moved exchangeRate here instead of dividing the other side to preserve more precision
            (borrowPart * _totalBorrow.elastic * _exchangeRate) /
                _totalBorrow.base;
    }

the same yieldBox.toAmount is called,

then a malicious user can frontrun the liquidation to call to manipulate the underlying stETH curve pool price to make the code falsely think that the borrow position is solvent and the share worth a lot and deny liquidation call

Repeated exploit of this issue leads to protocol insolvency.

Tools Used

Manual Review

Recommended Mitigation Steps

In LidoEthStrategy, when calculating the pending stETH balance, do not use the spot price from curve pool, instead, use TWAP oracle like other stragety implementation

Assessed type

Token-Transfer

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/Singularity.sol#L625 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/Singularity.sol#L92

Vulnerability details

Impact

Anyone can permissionlessly destroy the Singularity.sol contract.

Proof of Concept

In Singularity contrac, the code replies on the delegatecall to execute the module

   function _executeModule(
        Module _module,
        bytes memory _data
    ) private returns (bytes memory returnData) {
        bool success = true;
        address module = _extractModule(_module);

        (success, returnData) = module.delegatecall(_data);
        if (!success) {
            revert(_getRevertMsg(returnData));
        }
    }

this calls _extractModule extract a module contract

    function _extractModule(Module _module) private view returns (address) {
        address module;
        if (_module == Module.Borrow) {
            module = address(borrowModule);
        } else if (_module == Module.Collateral) {
            module = address(collateralModule);
        } else if (_module == Module.Liquidation) {
            module = address(liquidationModule);
        } else if (_module == Module.Leverage) {
            module = address(leverageModule);
        }
        if (module == address(0)) {
            revert("SGL: module not set");
        }

        return module;
    }

but the issue is, anyone can frontrun the init function to set a malicious module contract

liquidationModule = SGLLiquidation(_liquidationModule);
collateralModule = SGLCollateral(_collateralModule);
borrowModule = SGLBorrow(_borrowModule);
leverageModule = SGLLeverage(_leverageModule);

and then executes a function and call self-destruct in delegate call to destroy the Singularity.sol

POC:

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity >=0.8.0;

import {DSTest} from "ds-test/test.sol";
import {Vm} from "forge-std/Vm.sol";

library console {
    address constant CONSOLE_ADDRESS =
        address(0x000000000000000000636F6e736F6c652e6c6f67);

    function _sendLogPayload(bytes memory payload) private view {
        uint256 payloadLength = payload.length;
        address consoleAddress = CONSOLE_ADDRESS;
        assembly {
            let payloadStart := add(payload, 32)
            let r := staticcall(
                gas(),
                consoleAddress,
                payloadStart,
                payloadLength,
                0,
                0
            )
        }
    }

    function log() internal view {
        _sendLogPayload(abi.encodeWithSignature("log()"));
    }

    function logBool(bool p0) internal view {
        _sendLogPayload(abi.encodeWithSignature("log(bool)", p0));
    }
}

contract ContractA {
    address public owner;

    constructor() {
        owner = msg.sender;
    }

    function func1(address _addr) public returns (bool) {
        (bool success, bytes memory returndata) = _addr.delegatecall(
            abi.encodeWithSignature("func2()")
        );
        bool x = abi.decode(returndata, (bool));
        return x;
    }
}

contract ContractB {
    ContractC immutable con3;

    constructor() {
        con3 = new ContractC();
    }

    function func2() public returns (bool) {
        (bool success, bytes memory returndata) = address(con3).delegatecall(
            abi.encodeWithSignature("func3()")
        );
    }
}

contract ContractC {
    function func3() public {
        selfdestruct(payable(msg.sender));
    }
}

contract ContractTest is DSTest {
    Vm internal immutable vm = Vm(HEVM_ADDRESS);

    address payable[] internal users;

    ContractA a = new ContractA();
    ContractB b = new ContractB();

    function setUp() public {
        a.func1(address(b));
    }

    function isContract(address _addr) private returns (bool isContract) {
        uint32 size;
        assembly {
            size := extcodesize(_addr)
        }
        return (size > 0);
    }

    function testDestroy_POC() public {
        console.logBool(isContract(address(a)));
    }
}

note we need to use contractA -> delegateCall B and return bool data -> delegateCall C to destroy contract A to make sure the self-destroy works

In the POC above, the contract A is already destroyed and the logBool would log false because contract a has no code size.

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol guard the init function with onlyOwner or set the module contract directly in the constructor to avoid malicious self-destroy delegate call

Assessed type

call/delegatecall

Lines of code

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

Vulnerability details

Impact

AaveStragety.sol will not work if the swapper contract is changed because lack of token approval

Proof of Concept

In AaveStragety.sol

In the constructor, we set the the swappr contract

swapper = ISwapper(_multiSwapper);

and we give the token approval for the swapper

rewardToken.approve(_multiSwapper, type(uint256).max);

Then later when calling swapper.swap, we don't need to approve the token for swapper to use again

swapper.swap(swapData, minAmount, address(this), "");

However, if the admin change the swapper address, the compound function that calls swapper.swap will not working becaue when changing the swapper contract, no reward token spending allowance is given to the new swapper contract

    /// @notice Sets the Swapper address
    /// @param _swapper The new swapper address
    function setMultiSwapper(address _swapper) external onlyOwner {
        emit MultiSwapper(address(swapper), _swapper);
        swapper = ISwapper(_swapper);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol either given allowance before each swap or give max allowance when changing the swapper contract, we recommend the protocol give minimum allowance before the swap

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

the cost of manipulating TWAP in Optimism L2 network too low so TWAP oracle should not be used in optimism

Proof of Concept

The protocol intend to deploy on L2 network

according to

https://docs.tapioca.xyz/tapioca/information/supported-chains

Arbitrum (Host) Optimism Ethereum

Arbitrum is the host chain, the optimism and ethereum will be supported as well

The information provided by the Uniswap team, as documented in the Uniswap Oracle Integration on Layer 2 Rollups guide, primarily addresses the integration of Uniswap oracle on L2 Optimism. However, it is relevant to note that the same concerns apply to Arbitrum as well. Arbitrum's average block time is approximately 0.25 seconds, making it vulnerable to potential oracle price manipulation.

Oracles Integrations on Layer 2 Rollups

Optimism

On Optimism, every transaction is confirmed as an individual block. The block.timestamp of these blocks, however, reflect the block.timestamp of the last L1 block ingested by the Sequencer. For this reason, Uniswap pools on Optimism are not suitable for providing oracle prices, as this high-latency block.timestamp update process makes the oracle much less costly to manipulate. In the future, it's possible that the Optimism block.timestamp will have much higher granularity (with a small trust assumption in the Sequencer), or that forced inclusion transactions will improve oracle security. For more information on these potential upcoming changes, please see the Optimistic Specs repo. For the time being, usage of the oracle feature on Optimism should be avoided.

but such TWAP oracle is used as a source of oracle to determine the borrow / lending exchange rate and also the option token exercise price

also, the TWAP oracle is used to getOutputAmount in UniswapV3

the code is here

(int24 tick, ) = OracleLibrary.consult(pool, 60);

amountOut = OracleLibrary.getQuoteAtTick(
	tick,
	uint128(amountIn),
	tokenIn,
	tokenOut
);

this then fetch twap price from Uniswap pool

(
	int56[] memory tickCumulatives,
	uint160[] memory secondsPerLiquidityCumulativeX128s
) = IUniswapV3Pool(pool).observe(secondsAgos);

the timestamp is even hardcoded to 60, which make the cost of manipulating oracle lower in Optimism network

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend just fetch the oracle source from chainlink in optimism, or even fetch the chainlink price from all network

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Yearn Stragety tolerant 0 loss, which is too strict

Proof of Concept

When withdraw from Yearn Stragety

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

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

  @param maxLoss
        The maximum acceptable loss to sustain on withdrawal. Defaults to 0.01%.
        If a loss is specified, up to that amount of shares may be burnt to cover losses on withdrawal.

Yearn finance may implement yield farming stragety (such as lending / borrow in external platform) and subject to loss

settting 0 acceptable loss is too strict and can block withdraw if the yearn vault has loss

Tools Used

Manual Review

Recommended Mitigation Steps

do not hardcode 0 accetpable loss and make this parameter passable as function parameter

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

MEV bot can cause unfair liquidation when the BigBang Contract unpaused

Proof of Concept

BigBang.sol aims to implement lending / borrowing

user is expected to repay the debt by calling

    function repay(
        address from,
        address to,
        bool,
        uint256 part
    ) public notPaused allowedBorrow(from, part) returns (uint256 amount) {

repay can be paused and liquidation can be paused as well

 function liquidate(
        address[] calldata users,
        uint256[] calldata maxBorrowParts,
        ISwapper swapper,
        // @audit
        // this parameter does not work for more than 1 length?
        bytes calldata collateralToAssetSwapData
    ) external notPaused {

the same notPaused modifier applies to both function

the issue when the protocol pause the contract, the user is not able to repay the debt, the when user unpause the contract,

MEV bot can frontrun user's repay action and cause massive unfair liquidation and user does not have a chance to repay their debt when the contract is unpaused.

Tools Used

Manual

Recommended Mitigation Steps

Consider add a grace period to allow user repaying the debt when unpause the BigBang.sol contract

Assessed type

MEV

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/option-airdrop/AirdropBroker.sol#L535 https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/options/TapiocaOptionBroker.sol#L555

Vulnerability details

Impact

Lack of support for payment token that is more than 18 decimals in OptionBroker.sol and AirdroppedBroker.sol

Proof of Concept

When exercise the option,

the code try to get a OTC price from oracle and it is scaled by the payment token decimals,

however, according the

https://github.com/d-xo/weird-erc20#high-decimals

there are token that are more than 18 decimals

if the payment token is more than 18 decimals,

the code can revert in underflow

    function _getDiscountedPaymentAmount(
        uint256 _otcAmountInUSD,
        uint256 _paymentTokenValuation,
        uint256 _discount,
        uint256 _paymentTokenDecimals
    ) internal pure returns (uint256 paymentAmount) {
        // Calculate payment amount
        uint256 rawPaymentAmount = _otcAmountInUSD / _paymentTokenValuation;
        paymentAmount =
            rawPaymentAmount -
            muldiv(rawPaymentAmount, _discount, 100e4); // 1e4 is discount decimals, 100 is discount percentage

        paymentAmount = paymentAmount / (10 ** (18 - _paymentTokenDecimals));
    }

note the code:

18 - _paymentTokenDecimals

an example of this token would NEAR

https://etherscan.io/token/0x85f17cf997934a597031b2e18a9ab6ebd4b9f6a4#readContract

which is 24 decimals

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol does not make the assumption that the highest payment decimal is 18

Assessed type

Token-Transfer

Lines of code

https://github.com/Tapioca-DAO/tapiocaz-audit/blob/bcf61f79464cfdc0484aa272f9f6e28d5de36a8f/contracts/Balancer.sol#L288

Vulnerability details

Impact

Rebalance does not work if the Oft underlying token is ETH in Rebalancer.sol

Proof of Concept

In tapiocaz-audit\contracts\Balancer.sol,

we are calling the rebalance function

    function rebalance(
        address payable _srcOft,
        uint16 _dstChainId,
        uint256 _slippage,
        uint256 _amount,
        bytes memory _ercData
    )
        external
        payable
        onlyOwner
        onlyValidDestination(_srcOft, _dstChainId)
        onlyValidSlippage(_slippage)
    {

calling

//send
bool _isNative = ITapiocaOFT(_srcOft).erc20() == address(0);
if (_isNative) {
	if (msg.value <= _amount) revert FeeAmountNotSet();
	_sendNative(_srcOft, _amount, _dstChainId, _slippage);
} else {
	if (msg.value == 0) revert FeeAmountNotSet();
	_sendToken(_srcOft, _amount, _dstChainId, _slippage, _ercData);
}

if the ITapiocaOFT(_srcOft).erc20 is native token, we are expected to call _sendNative

_sendNative(_srcOft, _amount, _dstChainId, _slippage);

which calls routeETH.swapETH

    function _sendNative(
        address payable _oft,
        uint256 _amount,
        uint16 _dstChainId,
        uint256 _slippage
    ) private {
        if (address(this).balance < _amount) revert ExceedsBalance();

        routerETH.swapETH(
            _dstChainId,
            _oft, //refund
            abi.encodePacked(connectedOFTs[_oft][_dstChainId].dstOft),
            _amount,
            _computeMinAmount(_amount, _slippage)
        );
    }

However, if we see the stargate Router contract

https://etherscan.io/address/0x150f94B44927F078737562f0fcF3C95c01Cc2376#code#F1#L48

the code require user to supply ETH when calling swapETH, without supplying ETH, the swapETH will always revert

    // compose stargate to swap ETH on the source to ETH on the destination
    function swapETH(
        uint16 _dstChainId,                         // destination Stargate chainId
        address payable _refundAddress,             // refund additional messageFee to this address
        bytes calldata _toAddress,                  // the receiver of the destination ETH
        uint256 _amountLD,                          // the amount, in Local Decimals, to be swapped
        uint256 _minAmountLD                        // the minimum amount accepted out on destination
    ) external payable {
        require(msg.value > _amountLD, "Stargate: msg.value must be > _amountLD");

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol

calling

routerETH.swapETH{value: _amount + 1 wei}(
	_dstChainId,
	_oft, //refund
	abi.encodePacked(connectedOFTs[_oft][_dstChainId].dstOft),
	_amount,
	_computeMinAmount(_amount, _slippage)
);

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Compound is not called properly before Withdraw in Convex Stragety, result in either loss of reward or revert when withdrawal

Proof of Concept

In ConvexTricryptoStragety.sol#compound function,

the code did the following logic:

1. parse user's input parameter
2. check the balance before
3. claim rewrad
4. check the balance after
5. if the after > balance, swap all reward token to WETH
6. add WETH liquidity and stake the token to generate more reward

the logic is ok on the surface, but if the reward is claimed outside of the compound function, the balance before and after check will not include the already claimed reward

and the reward that is claimed outside of the compound is lost

When calling withdraw, the compound is called but there no reward claimed

for example, we can see the logic inside the _withdraw

	uint256 queued = wrappedNative.balanceOf(address(this));
	if (amount > queued) {

		// @audit
		// lose of extra reward
		// https://github.com/convex-eth/platform/blob/a5da3f127a321467a97a684c57970d2586520172/contracts/contracts/BaseRewardPool.sol#L238
		compound(bytes(""));
		uint256 lpBalance = rewardPool.balanceOf(address(this));

		// @audit
		// are you sure this is the exact lpBalance you can receive?
		rewardPool.withdrawAndUnwrap(lpBalance, false);
		uint256 calcWithdraw = lpGetter.calcLpToWeth(lpBalance);
		uint256 minAmount = calcWithdraw - (calcWithdraw * 50) / 10_000; //0.5%
		lpGetter.removeLiquidityWeth(lpBalance, minAmount);
	}

the compound("") is called but because o the supplied data is empty, the compound logic will not executed

the data.length is 0 and the function just return

   function compound(bytes memory data) public {
        if (data.length == 0) return;

then

  rewardPool.withdrawAndUnwrap(lpBalance, false);

actually help claim the reward

https://github.com/convex-eth/platform/blob/a5da3f127a321467a97a684c57970d2586520172/contracts/contracts/BaseRewardPool.sol#L238

   function withdrawAndUnwrap(uint256 amount, bool claim) public updateReward(msg.sender) returns(bool){

        //also withdraw from linked rewards
        for(uint i=0; i < extraRewards.length; i++){
            IRewards(extraRewards[i]).withdraw(msg.sender, amount);
        }
        
        _totalSupply = _totalSupply.sub(amount);
        _balances[msg.sender] = _balances[msg.sender].sub(amount);

        //tell operator to withdraw from here directly to user
        IDeposit(operator).withdrawTo(pid,amount,msg.sender);
        emit Withdrawn(msg.sender, amount);

        //get rewards too
        if(claim){
            getReward(msg.sender,true);
        }
        return true;
    }

Even the claim flag is set to false,

the extraRewards will still be transferred to the Convex Stragety contract, and this part of the reward is lost

why the reward is lost, because as the report mentioned in the begining,

we are using balance before -> claim reward -> balance after to track the claimed the reward

the extra reward claim happens outside so the balance counts towards "balance before" and will not be swapped to WETH in the compound function

for(uint i=0; i < extraRewards.length; i++){
	IRewards(extraRewards[i]).withdraw(msg.sender, amount);
}

or if the extraRewards[i] contract has insufficient reward token,

calling

IRewards(extraRewards[i]).withdraw(msg.sender, amount);

would result in revert and block withdrawal

Tools Used

Manual Review

Recommended Mitigation Steps

We recommend the protocol not using the balance before and balance after pattern when claiming the reward

just call claim and then check the rewardToken.balanceOf(address(this)) and swap all reward token available in the contract to WETH and add liquidity when compounding

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Loss of COMP reward in CompoundStragety.sol

Proof of Concept

In CompoundStragety.sol

the code support CETHER.sol integration

when deposit is called

we are calling

 cToken.mint{value: queued}();

when withdraw is called

we are calling withdraw, we are calling redeem to redeem CETHER

 cToken.redeem(toWithdraw);

However,

When supplying ETH and hold cETH (cTOKEN), the cTOKEN accures COMP reward

https://docs.compound.finance/v2/comptroller/#comp-distribution-speeds

The “COMP speed” unique to each market is an unsigned integer that specifies the amount of COMP that is distributed, per block, to suppliers and borrowers in each market. This number can be changed for individual markets by calling the _setCompSpeed method through a successful Compound Governance proposal. The following is the formula for calculating the rate that COMP is distributed to each supported market.

If we check the code, the logic is in Comptroller.sol

CETHER inherit from CToken

We are calling mintFresh in CToken

calling comptroller.mintAllowed

Inside the funtion mintAllowed, we have a function distributeSupplierComp

the logic distributeSupplierComp is here

    /**
     * @notice Calculate COMP accrued by a supplier and possibly transfer it to them
     * @param cToken The market in which the supplier is interacting
     * @param supplier The address of the supplier to distribute COMP to
     */
    function distributeSupplierComp(address cToken, address supplier) internal {

To claim the COMP reward, a user needs the logic to call claimCOMP

/**
 * @notice Claim all the comp accrued by holder in all markets
 * @param holder The address to claim COMP for
 */
function claimComp(address holder) public {
	return claimComp(holder, allMarkets);
}

there is a seperate function claimComp

  /**
     * @notice Claim all the comp accrued by holder in the specified markets
     * @param holder The address to claim COMP for
     * @param cTokens The list of markets to claim COMP in
     */
    function claimComp(address holder, CToken[] memory cTokens) public {
        address[] memory holders = new address[](1);
        holders[0] = holder;
        claimComp(holders, cTokens, true, true);
    }

even anyone can call claimComp above to help the CompoundStragety.sol claim the reward

the COMP reward still locked in the CompoundStragety.sol contract because there is lack of logic to handle the COMP token reward

Tools Used

Manual Review

Recommended Mitigation Steps

Adding the logic to claim the COMP reward and swap COMP reward to WETH / ETH

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

AaveStragety#withdraw and emergecyWithdraw can revert if the supply cap is reached or isFrozen flag is on when compounding

Proof of Concept

In AaveStragety#withdraw and AaveStragety#emergecyWithdraw

The general logic is:

1. first compounding the reward
2. withdraw from lending pool

the general logic in compounding is:

1. check claimable reward
2. claim reward
3. check cooldown period and reset cooldown period
4. swap AAVE token to WETH token
5. deposit (supply) WETH token to lending pool again

while this logic on the surface looks ok

the problem is that withdraw logic is tied to the deposit logic

When the asset reach supply cap or when the underlying market has the deposit feature disabled

the withdraw and emergencyWithdraw will keep revert and lock fund as well

Let us look at what is the code that can block deposit and then block withdraw

When calling LendingPool.deposit, we are calling SupplyLogic.executeSupply

then calling validateSupply

We need to put the logic of validate supply and validate withdraw side by side to better understanding the problem:

logic of validateSupply is here

  function validateSupply(
    DataTypes.ReserveCache memory reserveCache,
    DataTypes.ReserveData storage reserve,
    uint256 amount
  ) internal view {
    require(amount != 0, Errors.INVALID_AMOUNT);

    (bool isActive, bool isFrozen, , , bool isPaused) = reserveCache
      .reserveConfiguration
      .getFlags();
    require(isActive, Errors.RESERVE_INACTIVE);
    require(!isPaused, Errors.RESERVE_PAUSED);
    require(!isFrozen, Errors.RESERVE_FROZEN);

    uint256 supplyCap = reserveCache.reserveConfiguration.getSupplyCap();
    require(
      supplyCap == 0 ||
        ((IAToken(reserveCache.aTokenAddress).scaledTotalSupply() +
          uint256(reserve.accruedToTreasury)).rayMul(reserveCache.nextLiquidityIndex) + amount) <=
        supplyCap * (10 ** reserveCache.reserveConfiguration.getDecimals()),
      Errors.SUPPLY_CAP_EXCEEDED
    );
  }

logic of validateWithdraw logic is here

  function validateWithdraw(
    DataTypes.ReserveCache memory reserveCache,
    uint256 amount,
    uint256 userBalance
  ) internal pure {
    require(amount != 0, Errors.INVALID_AMOUNT);
    require(amount <= userBalance, Errors.NOT_ENOUGH_AVAILABLE_USER_BALANCE);

    (bool isActive, , , , bool isPaused) = reserveCache.reserveConfiguration.getFlags();
    require(isActive, Errors.RESERVE_INACTIVE);
    require(!isPaused, Errors.RESERVE_PAUSED);
  }

If AAVE admin pause or deactivate the pool, both deposit and withdraw will revert we can do nothing

but there are two additional check if the validateSupply logic

if the isFrozen flag is one, deposit (supply) revert

 require(!isFrozen, Errors.RESERVE_FROZEN);

If the supply cap for a asset is reached, deposit (supply) revert

   uint256 supplyCap = reserveCache.reserveConfiguration.getSupplyCap();
    require(
      supplyCap == 0 ||
        ((IAToken(reserveCache.aTokenAddress).scaledTotalSupply() +
          uint256(reserve.accruedToTreasury)).rayMul(reserveCache.nextLiquidityIndex) + amount) <=
        supplyCap * (10 ** reserveCache.reserveConfiguration.getDecimals()),
      Errors.SUPPLY_CAP_EXCEEDED

then when supply cap is reached or isFrozen flag is on, compounding will revert because the user cannot deposit asset to the lending pool

but because when withdrawing or emergencyWithdraw, compound is always called first, the withdraw function will revert as well

Tools Used

Manual Review

Recommended Mitigation Steps

When depositing, we need to handle the deposit gracefully, we can check if the supply cap is reached and if the isFrozen flag is on, and we can choose not to deposit the asset if we know deposit will revert

we can also choose to warp the lendingPool.deposit in try catch block to handle the revert gracefully

and we can choose not call compounding when calling emergency withdraw

Assessed type

Invalid Validation

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/Singularity.sol#L322 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLLeverage.sol#L117

Vulnerability details

Impact

Excessive sold asset is not handled properly in SGLLeverage#sellCollateral

Proof of Concept

In Singularity, user can sellCollateral

    /// @notice Lever down: Sell collateral to repay debt; excess goes to YB
    /// @param from The user who sells
    /// @param share Collateral YieldBox-shares to sell
    /// @param minAmountOut Mininal proceeds required for the sale
    /// @param swapper Swapper to execute the sale
    /// @param dexData Additional data to pass to the swapper
    /// @return amountOut Actual asset amount received in the sale
    function sellCollateral(
        address from,
        uint256 share,
        uint256 minAmountOut,
        ISwapper swapper,
        bytes calldata dexData
    ) external returns (uint256 amountOut) {
        bytes memory result = _executeModule(
            Module.Leverage,
            abi.encodeWithSelector(
                SGLLeverage.sellCollateral.selector,
                from,
                share,
                minAmountOut,
                swapper,
                dexData
            )
        );
        amountOut = abi.decode(result, (uint256));
    }

as the comment suggest, the Sell collateral to repay debt; excess goes to yield box

but when we are calling sell Collateral in SGLLeverage module

   uint256 shareOut;
        (amountOut, shareOut) = swapper.swap(
            swapData,
            minAmountOut,
            from,
            dexData
        );
        // As long as the ratio is correct, we trust `amountOut` resp.
        // `shareOut`, because all money received by the swapper gets used up
        // one way or another, or the transaction will revert.
        require(amountOut >= minAmountOut, "SGL: not enough");
        uint256 partOwed = userBorrowPart[from];
        uint256 amountOwed = totalBorrow.toElastic(partOwed, true);
        uint256 shareOwed = yieldBox.toShare(assetId, amountOwed, true);
        if (shareOwed <= shareOut) {
            _repay(from, from, false, partOwed);
        } else {
            //repay as much as we can
            uint256 partOut = totalBorrow.toBase(amountOut, false);
            _repay(from, from, false, partOut);
        }

as we can see, the swapped amount is amountOut

if the case of shareOwed <= shareOut

if (shareOwed <= shareOut) {
	_repay(from, from, false, partOwed);
}

the amount of partOwed is repaid

but the excessive amount (amountOut - partOwed) is not properly handled and deposit into yield box

Tools Used

Manual Review

Recommended Mitigation Steps

Consider add the excessive amount sold asset to yield box in SGLLeverage#sellCollateral

Assessed type

Token-Transfer

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/glp/GlpStrategy.sol#L176

Vulnerability details

Impact

Lack of slippage control when mintAndStakeGLP

Proof of Concept

In GlpStragety.sol, after claim the WETH from call _callimRewards:

    function _claimRewards() private {
        gmxRewardRouter.handleRewards({
            _shouldClaimGmx: true,
            _shouldStakeGmx: false,
            _shouldClaimEsGmx: true,
            _shouldStakeEsGmx: false,
            _shouldStakeMultiplierPoints: true,
            _shouldClaimWeth: true,
            _shouldConvertWethToEth: false
        });
    }

we set the shouldClaimWETH flag to true so the GlpStragety will receive WETH

then the function _buyGLP is called

function _buyGlp() private {
	uint256 wethAmount = weth.balanceOf(address(this));
	uint256 _feesPending = feesPending;
	if (wethAmount > _feesPending) {
		wethAmount -= _feesPending;
		uint256 fee = (wethAmount * FEE_BPS) / 10_000;
		feesPending = _feesPending + fee;
		wethAmount -= fee;

		weth.approve(address(glpManager), wethAmount);
		// @audit
		// slippage control
		glpRewardRouter.mintAndStakeGlp(address(weth), wethAmount, 0, 0);
	}
}

note the function call:

glpRewardRouter.mintAndStakeGlp(address(weth), wethAmount, 0, 0);

we are trying to use the WETH reward we get to add liquidity and mint GLP

https://arbiscan.io/address/0xB95DB5B167D75e6d04227CfFFA61069348d271F5#code#F1#L129

    function mintAndStakeGlp(address _token, uint256 _amount, uint256 _minUsdg, uint256 _minGlp) external nonReentrant returns (uint256) {
        require(_amount > 0, "RewardRouter: invalid _amount");

        address account = msg.sender;
        uint256 glpAmount = IGlpManager(glpManager).addLiquidityForAccount(account, account, _token, _amount, _minUsdg, _minGlp);
        IRewardTracker(feeGlpTracker).stakeForAccount(account, account, glp, glpAmount);
        IRewardTracker(stakedGlpTracker).stakeForAccount(account, account, feeGlpTracker, glpAmount);

        emit StakeGlp(account, glpAmount);

        return glpAmount;
    }

as we can see, the parameter minUsdg and the parameter minGLP are both set to 0

https://arbiscan.io/address/0x3963FfC9dff443c2A94f21b129D429891E32ec18#code#F1#L209

the GLP token is only minted when add liquidity

    function _addLiquidity(address _fundingAccount, address _account, address _token, uint256 _amount, uint256 _minUsdg, uint256 _minGlp) private returns (uint256) {
        require(_amount > 0, "GlpManager: invalid _amount");

        // calculate aum before buyUSDG
        uint256 aumInUsdg = getAumInUsdg(true);
        uint256 glpSupply = IERC20(glp).totalSupply();

        IERC20(_token).safeTransferFrom(_fundingAccount, address(vault), _amount);
        uint256 usdgAmount = vault.buyUSDG(_token, address(this));
        require(usdgAmount >= _minUsdg, "GlpManager: insufficient USDG output");

        uint256 mintAmount = aumInUsdg == 0 ? usdgAmount : usdgAmount.mul(glpSupply).div(aumInUsdg);
        require(mintAmount >= _minGlp, "GlpManager: insufficient GLP output");

        IMintable(glp).mint(_account, mintAmount);

        lastAddedAt[_account] = block.timestamp;

        emit AddLiquidity(_account, _token, _amount, aumInUsdg, glpSupply, usdgAmount, mintAmount);

        return mintAmount;
    }

as we can see, if the minGLP and minUsdg are both set to 0

the slippage check

   require(usdgAmount >= _minUsdg, "GlpManager: insufficient USDG output");

and

   require(mintAmount >= _minGlp, "GlpManager: insufficient GLP output");

both becomes ineffective

the GLP mitned amount is calculated as:

 uint256 mintAmount = aumInUsdg == 0 ? usdgAmount : usdgAmount.mul(glpSupply).div(aumInUsdg);

and this is purely depends on the glpSupply and aumInUsdg (comes from vault.buyUSDG(_token, address(this));)

then a very suboptimal amount of the GLP token can be minted because of the lack of slippage check, especically if other transaction lands before the mintAndStakeGLP transaction that inflate the value of "aumInUsdg"

Tools Used

Manual Review

Recommended Mitigation Steps

Let user specify minimum amount of GLP minted,

either the parameter _minUsdg or _minGlp needs to be set to a non zero to avoid the loss from slippage

Assessed type

Token-Transfer