Tapioca DAO - c7e7eff's results

Lines of code

https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L438-L441 https://github.com/boringcrypto/BoringSolidity/blob/78f4817d9c0d95fe9c45cd42e307ccd22cf5f4fc/contracts/BoringFactory.sol#L61

Vulnerability details

Summary

Markets the have Penrose set as owner and are added to it rather than directly deployed via Penrose will not be manageable by Penrose as the owner only functions will be blocked by Penrose's executeMarketFn() function. This is due to a critical parameter (masterContractOf) not being set for these markets when adding them.

Vulnerability Detail

Penrose is meant to be set as the owner of the Singularity and BigBang markets, either by deploying markets directly via the registerBigBang() and registerSingularity() functions or by adding already existing market contracts via the addBigBang() and addSingularity() functions.

It has been made clear in DMs that the market contracts will not be directly deployed via Penrose but rather they will be added via the addBigBang() and addSingularity() functions.

The executeMarketFn() function in the Penrose contract enables it's owner to execute owner functions on the registered markets. Before it does so a require statement checks that the targeted market contract has its master contract registered in Penrose. This is a safety feature so that it will never call an untrusted contract/proxy.

    function executeMarketFn(
        address[] calldata mc,
        bytes[] memory data,
        bool forceSuccess
    ) external onlyOwner notPaused
        returns (bool[] memory success, bytes[] memory result)
    {
		...
        for (uint256 i = 0; i < len; ) {
            require(
                isSingularityMasterContractRegistered[
                    masterContractOf[mc[i]]
                ] || isBigBangMasterContractRegistered[masterContractOf[mc[i]]],
                "Penrose: MC not registered"
            );
            (success[i], result[i]) = mc[i].call(data[i]);
	        ...
        }
    }

Note that to find the master contract of the targeted market contract it uses the masterContractOf[] mapping

In the register functions this is set in the underlying deploy()function of the BoringFactory contract:

    function deploy(
        address masterContract,
        bytes calldata data,
        bool useCreate2
    ) public payable returns (address cloneAddress) {
		...
        masterContractOf[cloneAddress] = masterContract;
        ...
    }
}

However when a market has been added via the addBigBang() or addSingularity() functions the masterContractOf[] is not set.

    function addSingularity(
        address mc,
        address _contract
    ) external onlyOwner registeredSingularityMasterContract(mc) {
        isMarketRegistered[_contract] = true;
        clonesOf[mc].push(_contract);
        emit RegisterSingularity(_contract, mc);
    }

Impact

If a market is added via one of the addBigBang() or addSingularity() functions and the ownership of the market has been changed to the Penrose contract, owner only functions cannot be called on the market contract anymore.

Tool used

Manual Review

Recommendation

Set the masterContractOf for the added market in addBigBang() and addSingularity() functions.

Assessed type

Other

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L141-L167 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L118-L125 https://github.com/curvefi/curve-contract/blob/b0bbf77f8f93c9c5f4e415bce9cd71f0cdee960e/contracts/pools/steth/StableSwapSTETH.vy#L510-L563 https://github.com/Tapioca-DAO/YieldBox/blob/f5ad271b2dcab8b643b7cf622c2d6a128e109999/contracts/YieldBox.sol#L280-L291

Vulnerability details

Summary

The Lido strategy uses Curve as an oracle for the funds held in stETH. As Curve is susceptible to read-only reentrancy attack this can be used to skew the price of stETH and the estimation of the funds in the strategy.

An attacker can deposit ETH into the strategy's queue its limit. Then with a 0% fee flashloan from Balancer, add (ETH) liquidity and make an imbalanced liquidity withdraw (1 wei ETH and the rest in stETH) increasing the stETH/ETH price.

The strategy will then estimate the value of the stETH in the strategy to high, which will enable the attacker to withdraw from the queue at an inflated price. The attack is limited to the size of the queue as going beyond that would call Curve's exchange() function which does have reentrancy protection.

The remaining Asset tokens can be withdraw after the flash loan at normal prices as profit.

Vulnerability Detail

When withdrawing from the Lido strategy it first checks whether the request amount is less than the total amount currently in the strategy (_currentBalance())

    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 obtainedEth = curveStEthPool.exchange(
                1,
                0,
                toWithdraw,
                minAmount
            );


            INative(address(wrappedNative)).deposit{value: obtainedEth}();
        }
		...
		wrappedNative.safeTransfer(to, amount);
    }

Note that if the amount requested is less than the current balance of WETH, the withdraw won't call into the Curve pool's state changing exchange() function.

The theoretical amount returned by the Curve pool is queried by using the get_dy() view function on the pool. Curve is actually used as an oracle here to calculate the value of the funds in the strategy.

    function _currentBalance() internal view override returns (uint256 amount) {
        uint256 stEthBalance = stEth.balanceOf(address(this));
        uint256 calcEth = stEthBalance > 0
            ? curveStEthPool.get_dy(1, 0, stEthBalance)
            : 0;
        uint256 queued = wrappedNative.balanceOf(address(this));
        return calcEth + queued;
    }

The stETH/ETH curve pool is susceptible to read-only reentrancy, as is well documented by ChainSecurity here: https://chainsecurity.com/heartbreaks-curve-lp-oracles/

Although the blog mentions the use of get_virtual_price() the get_dy() function is just as dependent on the pool's balance to determine the amount of ETH returned:

def get_dy(i: int128, j: int128, dx: uint256) -> uint256:
    xp: uint256[N_COINS] = self._balances()
    x: uint256 = xp[i] + dx
    y: uint256 = self.get_y(i, j, x, xp)
    dy: uint256 = xp[j] - y - 1
    fee: uint256 = self.fee * dy / FEE_DENOMINATOR
    return dy - fee

Using the get_dy() function when the pool is imbalanced (e.g. due to large LP removal) results in a wrong calculation of the value of stETH held in the Lido strategy (expressed as ETH). Thereby overestimating the total value of the strategy and transferring to much of the queued WETH for the amount of strategy tokens burnt.

function _withdrawFungible(...)
internal returns (uint256 amountOut, uint256 shareOut) {
        uint256 totalAmount = _tokenBalanceOf(asset);
        if (share == 0) {
            share = amount._toShares(totalSupply[assetId], totalAmount, true);
        } else {
            amount = share._toAmount(totalSupply[assetId], totalAmount, false);
        }
        _burn(from, assetId, share);
        asset.strategy.withdraw(to, amount);
        return (amount, share);
    }

Impact

As long as the requested amount is lower than the current queued amount (which can be provide by the attacker themselves) an attacker could use a flash loan to trigger the imbalance and get more (W)ETH for the amount of Asset tokens than they should be able to.

Similar findings have been published here: https://github.com/sherlock-audit/2022-12-sentiment-judging/issues/7 and here: https://github.com/sherlock-audit/2022-12-sentiment-judging/issues/20

As the ETH/stETH pool has a lot of liquidity ($360,000,000) the impact of the attack is limited however (hence the Medium level). The economical viability of the attack also depends on the queue/threshold configured and the amount of stETH held in the strategy. (higher threshold and more stETH held increases the viability)

Note however that the cost of the attack is low as Balancer flash loans have 0% fee set (and 80k ETH available). Therefor an attacker only needs to provide the gas needed.

The extent of this vulnerability is not limited to the Lido strategy itself though. It also impacts all functions of Yieldbox that rely on _currentBalance():

	depositAsset()
	depositETHAsset()
	assetTotals()
	toShare()
	toAmount()
	amountOf()

And hence any contracts that rely on any of those functions throughout the code base. Notable examples include:

TapiocaOptionLiquidityProvision
BigBang
Singularity
SGLCollateral
SGLLeverage
USD0
MagnetarV2
MagnetarMarketModule
LiquidationQueue
UniUsdoToWethBidder
CurveStableToUsdoBidder
SGLLendingCommon

It also impacts other code locations where view functions of Curve are called directly. For instance ARBTriCryptoOracle.soluses the get_virtual_price()

function _get() internal view returns (uint256 _maxPrice) {
        uint256 _vp = TRI_CRYPTO.get_virtual_price();
        ...
}

Tool used

Manual Review

Recommendation

The simplest way to detect the Curve's read-only reentrancy is by calling a reentrancy protected function on the Curve pool, e.g. remove_liquidity() with zero amounts.

    function _withdraw(
        address to,
        uint256 amount
    ) internal override nonReentrant {
+       uint256[2] calldata amounts;
+       ICurvePool(token).remove_liquidity(0, amounts);
        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

This will revert if the Curve pool is locked and succeed when not locked without actually altering state in the Curve pool.

Assessed type

Oracle

Lines of code

https://github.com/Tapioca-DAO/tap-token-audit/blob/59749be5bc2286f0bdbf59d7ddc258ddafd49a9f/contracts/governance/twTAP.sol#L265

Vulnerability details

Summary

In both twTAP and tOB when calculating the cumulative it is purely based on the duration chosen by the participant (not the amount). As the cumulative is easy to increase but very hard to decrease early participants using a low amount can force a high cumulative on future participants thereby making sure future participants get little discount or voting power. The result is that every future participant is forced the minimal discount on their participation, while the attacker is still given the high discount/voting power.

Vulnerability Detail

twTap's participate function calculates the weight the lockup of TAP tokens via computeMagnitude() according to the which is based on the TAML whitepaper. The sole input for this calculation however are the time the participant wants to lockup their tokens and the current pool cumulative (which itself is based on previous magnitudes). As long as the amount meets the minimum requirement of 0.1 of the pool it does not play a role in the calculation of the magnitude and more importantly the the amplitude adds to the cumulative.

The problem is that when the pool is small (like at genesis) anyone can influence the cumulative (and hence the future rewards for other participants) by just locking up 0.1% of the pool for an extremely long time.

Take as example when the first user participates with their chosen amount of TAP for a reasonable time ($100k for 2 years). Then immediately after they lock up just the minimum amount of $100 (0.1% of the current pool) for 1,000,000,000 years. This is what the average magnitude, cumulative and discount would look like for future participants that lockup for 10 years. The net discount is set to the minimum of 5% for all future participants https://docs.google.com/spreadsheets/d/13qmSJAgjuKbBfB7QjIdJz0ltLC_63fYPhq4Auf8V_oM/edit#gid=0

Note this is just the standard spreadsheet provided by the team but with modified weights chosen by the participants. https://docs.google.com/spreadsheets/d/1mXloSTPfsKExydOo_2fqjYdBYvJY7g2nn3lZl4QOp-g/edit#gid=0

Impact

As confirmed in DM by the team "the twAML system is weak at genesis" and more in general whenever the pool is low in liquidity resulting in one user being able to get an outsized amount of the rewards/discounts to the cost of future participants. As twTAP is considered the governance token this constitutes a governance attack.

Tool used

Manual Review

Recommendation

Include the amount locked for the duration when calculating the weight/magnitude and cumulative. Depending on how you want to balance the importance of time vs amount this might be a simple multiplication or a more complex combination of both.

Assessed type

Governance

Lines of code

https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/contracts/lido/LidoEthStrategy.sol#L150-L157 https://github.com/curvefi/curve-contract/blob/b0bbf77f8f93c9c5f4e415bce9cd71f0cdee960e/contracts/pools/steth/StableSwapSTETH.vy#L431-L440 https://github.com/Tapioca-DAO/tapioca-yieldbox-strategies-audit/blob/05ba7108a83c66dada98bc5bc75cf18004f2a49b/test/fork/lidoStrategy-fork.test.ts#L198-L207

Vulnerability details

Summary

The Curve function used to exchange stETH for ETH takes a parameter dx which is interpreted as the amount of token 'in'. The Lido strategy however specifies the required (W)ETH. This can lead to loss of funds for the user or reverted transactions (meaning locked funds) in the current code base.

Vulnerability Detail

In the Lido strategy the amount to be exchanged is calculated as the difference between the amount requested and the current amount queued. These amounts are expressed as (W)ETH.

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

It would then further be converted to WETH and only the requested amount would be transferred to the user.

When exchanging stETH for ETH in the Curve pool however the exchange() function specifies the dx as amount of the sent token (ie. stETH), not the amount of token to be received (ETH).

def exchange(i: int128, j: int128, dx: uint256, min_dy: uint256) -> uint256:
    """
    @notice Perform an exchange between two coins
    @dev Index values can be found via the `coins` public getter method
    @param i Index value for the coin to send
    @param j Index valie of the coin to recieve
    @param dx Amount of `i` being exchanged
    @param min_dy Minimum amount of `j` to receive
    @return Actual amount of `j` received
    """

As the exchange rate of stETH/ETH fluctuates around 1:1 the received amount will sometimes be to much and other times not enough. When stETH/ETH is above 1 the excess WETH received would just stay in the strategy as queued amount and only the requested amount would be transferred to the user.

Historically however this rate has been mostly below 1 so the amount of ETH received from the exchange would be less than the amount requested. Therefor the user would receive less than they should Due to another bug the transaction would with the current code revert due to this require statement). Note that this bug is not detected by any tests as the only test withdrawing from the strategy beyond the queue expects the withdrawal to revert.

Impact

When stETH/ETH exchange rate is below 1 the user would receive less than they should. Due to the mentioned bug in the current implementation the transaction would actually revert and the user would be unable to withdraw.

Tool used

Manual Review

Recommendation

Specify the requested amount (minus the queue) as the minAmount and specify a higher amount for toWithdraw (ie. an extra %). Any amount over the minAmount will stay in the queue as the user will only receive the requested amount in the transfer. Note that the extra percentage on the toWithdraw depends on the exchange rate of stETH/ETH and has in some extreme case been as high as 7% (June 9th 2022). This would mean the WETH transfer to the user would still revert in these cases.

There doesn't seem to be a way to query the amount of stETH necessary to receive a certain amount of WETH in Curve. Simply reversing the token Ids in get_dy won't be accurate due to the asymmetric nature of the curve used. As an alternative solution this service could be provided in the front-end (iteratively query get_dy until the requested amount is reached) and convey the amount in the withdraw request only as a hint to the amount required (and allow it to exceed the fixed percentage).

Assessed type

Other