Sushi Trident contest phase 1 - broccoli's results

Handle

broccoli

Vulnerability details

Flash swap call back prior to transferring tokens in indexPool

Impact

In the IndexPool contract, flashSwap does not work. The callback function is called prior to token transfer. The sender won't receive tokens in the callBack function. ITridentCallee(msg.sender).tridentSwapCallback(context);

Flashswap is not implemented correctly. It may need a migration to redeploy all indexPools if the issue is found after main-net launch. I consider this a high-risk issue.

Proof of Concept

IndexPool.sol#L196-L223

        ITridentCallee(msg.sender).tridentSwapCallback(context);
        // @dev Check Trident router has sent `amountIn` for skim into pool.
        unchecked { // @dev This is safe from under/overflow - only logged amounts handled.
            require(_balance(tokenIn) >= amountIn + inRecord.reserve, "NOT_RECEIVED");
            inRecord.reserve += uint120(amountIn);
            outRecord.reserve -= uint120(amountOut);
        }
        _transfer(tokenOut, amountOut, recipient, unwrapBento);

Tools Used

None

Recommended Mitigation Steps

        _transfer(tokenOut, amountOut, recipient, unwrapBento);
        ITridentCallee(msg.sender).tridentSwapCallback(context);
        // @dev Check Trident router has sent `amountIn` for skim into pool.
        unchecked { // @dev This is safe from under/overflow - only logged amounts handled.
            require(_balance(tokenIn) >= amountIn + inRecord.reserve, "NOT_RECEIVED");
            inRecord.reserve += uint120(amountIn);
            outRecord.reserve -= uint120(amountOut);
        }

Handle

broccoli

Vulnerability details

Index Pool always swap to Zero

Impact

When an Index pool is initiated with two tokens A: B and the weight rate = 1:2, then no user can buy token A with token B.

The root cause is the error in pow. It seems like the dev tries to implement Exponentiation by squaring. IndexPool.sol#L286-L291

function _pow(uint256 a, uint256 n) internal pure returns (uint256 output) {
  output = n % 2 != 0 ? a : BASE;
  for (n /= 2; n != 0; n /= 2) a = a * a;
  if (n % 2 != 0) output = output * a;
}

There's no bracket for for.

The IndexPool is not functional. I consider this is a high-risk issue.

Proof of Concept

When we initiated the pool with 2:1.

deployed_code = encode_abi(["address[]","uint136[]","uint256"], [
    (link.address, dai.address),
    (2*10**18,  10**18),
    10**13
])

No one can buy dai with link.

# (address tokenIn, address tokenOut, address recipient, bool unwrapBento, uint256 amountIn)
previous_balance = bento.functions.balanceOf(dai.address, admin).call()
swap_amount =  10**18

bento.functions.transfer(link.address, admin, pool.address, swap_amount).transact()
pool.functions.swap(
    encode_abi(
        ['address', 'address', 'address', 'bool', 'uint256'],
        [link.address, dai.address, admin, False, swap_amount]
    )
).transact()
current_balance = bento.functions.balanceOf(dai.address, admin).call()
token_received = current_balance - previous_balance
# always = 0
print(token_received)

Tools Used

None

Recommended Mitigation Steps

The brackets of for were missed.

    function _pow(uint256 a, uint256 n) internal pure returns (uint256 output) {
        output = n % 2 != 0 ? a : BASE;
        for (n /= 2; n != 0; n /= 2) {
            a = a * a;
            if (n % 2 != 0) output = output * a;
        }
    }

Handle

broccoli

Vulnerability details

Impact

In the IndexPool contract, pow is used in calculating price. IndexPool.sol#L255-L266 However, Pow is easy to cause overflow. If the weightRatio is large (e.g. 10), there's always overflow.

Lp providers can still provide liquidity to the pool where no one can swap. All pools need to redeploy. I consider this a high-risk issue.

Proof of concept

It's easy to trigger this bug by deploying a 1:10 IndexPool.

    deployed_code = encode_abi(["address[]","uint136[]","uint256"], [
        (link.address, dai.address),
        (10**18, 10 * 10**18),
        10**13
    ])
    tx_hash = master_deployer.functions.deployPool(index_pool_factory.address, deployed_code).transact()

Transactions would be reverted when buying link with dai.

Tools Used

None

Recommended Mitigation Steps

The weightRatio is an 18 decimals number. It should be divided by (BASE)^exp. The scale in the contract is not consistent. Recommend the dev to check all the scales/ decimals.

Handle

broccoli

Vulnerability details

Impact

The indexPool mint INIT_POOL_SUPPLY to address 0 in the constructor. However, the value of the burned lp is decided by the first lp provider. According to the formula IndexPool.sol#L106.

AmountIn = first_lp_amount / INIT_POOL_SUPPLY and the burned lp worth = AmountIn * (INIT_POOL_SUPPLY) / (first_lp_amount + INIT_POOL_SUPPLY). If a pool is not initialized with optimal parameters, it would be a great number of tokens been burn. All lp providers in the pool would receive less profit.

The optimal parameter is 10**8. It's likely no one would initialize with 10**8 wei in most pools. I consider this is a high-risk issue.

Proof of concept

There are two scenarios that the first lp provider can do. The lp provider provides the same amount of token in both cases. However, in the first scenario, he gets about 10 ** 18 * 10**18 lp while in the other scenario he gets 100 * 10**18 lp.

deposit_amount = 10**18
bento.functions.transfer(link.address, admin, pool.address, deposit_amount).transact()
bento.functions.transfer(dai.address, admin, pool.address, deposit_amount).transact()
pool.functions.mint(encode_abi(
    ['address', 'uint256'],
    [admin, 10**8] # minimum
)).transact()
pool.functions.mint(encode_abi(
    ['address', 'uint256'],
    [admin, 10000000000009999 * 10** 20]
)).transact()

deposit_amount = 10**18
bento.functions.transfer(link.address, admin, pool.address, deposit_amount).transact()
bento.functions.transfer(dai.address, admin, pool.address, deposit_amount).transact()
pool.functions.mint(encode_abi(
    ['address', 'uint256'],
    [admin, deposit_amount * 100]
)).transact()

Recommended Mitigation Steps

Recommend to handle INIT_POOL_SUPPLY in uniswap-v2's way. Determine an optimized parameter for the user would be a better UX design.

Handle

broccoli

Vulnerability details

Impact

When an lp provider deposits an imbalance amount of token, a swap fee is applied. HybridPool uses the same _nonOptimalMintFee as constantProductPool; however, since two pools use different AMM curve, the ideal balance is not the same. ref: StableSwap3Pool.vy#L322-L337

Stable swap Pools are designed for 1B+ TVL. Any issue related to pricing/fee is serious. I consider this is a high-risk issue

Proof of Concept

StableSwap3Pool.vy#L322-L337

HybridPool.sol#L425-L441

Tools Used

None

Recommended Mitigation Steps

Calculate the swapping fee based on the stable swap curve. Please refer to StableSwap3Pool.vy#L322-L337.

Handle

broccoli

Vulnerability details

Impact

In the IndexPool contract, the first lp providers have to deposit the same amount of tokens. This creates arbitrage space.

If the deployer tries to deploy a BTC/DAI pool. S/He has to initialize the BTC pool with BTC price = 1.

I consider this is a medium-risk issue

Proof of Concept

IndexPool.sol#L95-L117

The logic that decides the amountIn of the first lp provider:

            // @dev If token balance is '0', initialize with `ratio`.
            uint120 amountIn = reserve != 0 ? uint120(_mul(ratio, reserve)) : ratio;

amountIn = ratio for all tokens.

Tools Used

None

Recommended Mitigation Steps

I would say it is a bad UX design than a bug. I consider allowing the first lp provider to decide the price is a better choice.

Handle

broccoli

Vulnerability details

Impact

The difference function of MathUtils is incorrect. Without a return statement in the if bracket, the function always returns diff = b - a, causing difference(x + 1, x) to be uint(-1), and thus within(x + 1, x) is false. The within function is used to in the _getY function and for calculating the D invariant, and the incorrect result of within could cause inaccurate amount of tokens being swaped, or inaccurate pool liquidity being used.

Proof of Concept

Referenced code: MathUtils.sol#L22-L29

Recommended Mitigation Steps

Add a return diff; after the line diff = a - b.

Handle

broccoli

Vulnerability details

Impact

It is possible to overflow the addition in the balance check (i.e., _balance(tokenIn) >= amountIn + reserve) in the mint function by setting the amountIn to a large amount. As a result, the attacker could gain a large number of LP tokens by not even providing any liquidity. The attacker's liquidity would be much greater than any other LPs, causing him could effectively steal others' funds by burning his liquidity (since the funds he receives are proportional to his liquidity).

Proof of Concept

PoC: Link to PoC

Referenced code: IndexPool.sol#L110

Recommended Mitigation Steps

Consider removing the uncheck statement to prevent integer overflows from happening.

Handle

broccoli

Vulnerability details

Impact

The _computeSingleOutGivenPoolIn function of IndexPool uses the _pow function to calculate tokenOutRatio with the exponent in WAD (i.e., in 18 decimals of precision). However, the _pow function assumes that the given exponent n is not in WAD. (for example, _pow(5, BASE) returns 5 ** (10 ** 18) instead of 5 ** 1). The misuse of the _pow function could causes an integer overflow in the _computeSingleOutGivenPoolIn function and thus prevent any function from calling it.

Proof of Concept

Referenced code: IndexPool.sol#L279

Recommended Mitigation Steps

Change the _pow function to the _compute function, which supports exponents in WAD.

Handle

broccoli

Vulnerability details

Impact

The _computeSingleOutGivenPoolIn function of IndexPool uses the raw multiplication (i.e., *) to calculate the zaz variable. However, since both (BASE - normalizedWeight) and _swapFee are in WAD, the _mul function should be used instead to calculate the correct value of zaz. Otherwise, zaz would be 10 ** 18 times larger than the expected value and causes an integer underflow when calculating amountOut. The incorrect usage of multiplication prevents anyone from calling the function successfully.

Proof of Concept

Referenced code: IndexPool.sol#L282

Recommended Mitigation Steps

Change (BASE - normalizedWeight) * _swapFee to _mul((BASE - normalizedWeight), _swapFee).

Handle

broccoli

Vulnerability details

Impact

An attacker can call the bento.harvest function during the callback function of a flash swap of the HybridPool to reduce the number of input tokens that he has to pay to the pool, as long as there is any unrealized profit in the strategy contract of the underlying asset.

Proof of Concept

1. The HybridPool accounts for the reserve and balance of the pool using the bento.toAmount function, which represents the actual amount of assets that the pool owns instead of the relative share. The value of toAmount could increase or decrease if the bento.harvest function is called (by anyone), depending on whether the strategy contract earns or loses money.
2. Supposing that the DAI strategy contract of Bento has a profit not accounted for yet. To account for the profit, anyone could call harvest on Bento with the corresponding parameters, which, as a result, increases the elastic of the DAI token.
3. Now, an attacker wants to utilize the unrealized profit to steal funds from a DAI-WETH hybrid pool. He calls flashSwap to initiate a flash swap from WETH to DAI. First, the pool transfers the corresponding amount of DAI to him, calls the tridentSwapCallback function on the attacker's contract, and expects that enough DAI is received at the end.
4. During the tridentSwapCallback function, the attacker calls bento.harvest to realize the profit of DAI. As a result, the pool's bento.toAmount increases, and the amount of DAI that the attacker has to pay to the pool is decreased. The attacker could get the same amount of ETH but paying less DAI by exploiting this bug.

Referenced code: HybridPool.sol#L218-L220 HybridPool.sol#L249-L250 HybridPool.sol#L272-L285 BentoBoxV1Flat.sol#L1105 BentoBoxV1Flat.sol#L786-L792 BentoBoxV1Flat.sol#L264-L277

Recommended Mitigation Steps

Consider not using bento.toAmount to track the reservers and balances, but use balanceOf instead (as done in the other two pools).

Handle

broccoli

Vulnerability details

Adding imbalanced liquidity earns extra rewards

Impact

When a user provides liquidity with unbalanced balance. It should be the same as swapping tokens and adding lp. However, the liquidity the users get is calculated as follow:

 uint256 computed = TridentMath.sqrt((balance0 - fee0) * (balance1 - fee1));
uint256 k = TridentMath.sqrt(uint256(_reserve0) * _reserve1);
liquidity = ((computed - k) * _totalSupply) / k;

The user can take a share of the swapping fee he's paying. It's like first add the liquidity then swap the tokens.

It may be a design choice. However, the pool is designed to hold billions of dollars. Any extra fee would be amplified. I consider this is a medium-risk issue.

ConstantProductPool and HybridPool have the same issue. ConstantProductPool.sol#L83-L120

HybridPool.sol#L115-L139

Proof of Concept

I noticed that imbalanced mint and burnSingle may sometimes have a better price than swapping. It may be a lot of issues involved.

Tools Used

None

Recommended Mitigation Steps

    // Note: what is the optimal value of computed
    uint256 computed = TridentMath.sqrt((balance0) * (balance1));
    
    // The user should get the lp based on post-swap-fee reserve.
    uint256 k = TridentMath.sqrt(uint256(_reserve0 + fee) * _reserve1 + fee);
    liquidity = ((computed - k) * _totalSupply) / k;

Handle

broccoli

Vulnerability details

Impact

TridentRouter is easy to fail when trying to provide liquidity to an index pool.

Users would not get extra lp if they are not providing lp at the pool's spot price. It's the same design as uniswap v2. However, uniswap's v2 handle's the dirty part.

UniswapV2Router02.sol#L61-L76 Users would not lose tokens if they use the router.

However, the router wouldn't stop users from transferring extra tokens. TridentRouter.sol#L168-L190

Second, the price would possibly change when the transaction is confirmed. This would be reverted in the index pool.

Users would either transfer extra tokens or fail. I consider this is a medium-risk issue.

Proof of Concept

TridentRouter.sol#L168-L190

A possible scenario:

There's a BTC/USD pool. BTC = 50000 USD.

1. A user sends a transaction to transfer 1 BTC and 50000 USD.
2. After the user send a transaction, a random bot buying BTC with USD.
3. The transaction at step 1 is mined. Since the BTC price is not 50000 USD, the transaction fails.

Tools Used

None

Recommended Mitigation Steps

Please refer to the uniswap v2 router. UniswapV2Router02.sol#L61-L76

The router should calculate the optimal parameters for users.

Handle

broccoli

Vulnerability details

Title: Revert the transaction if convergence does not happen in HybridPool

Impact

The _getY and _computeLiquidityFromAdjustedBalances functions use iterative approximations to calculate the output token amount or the liquidity invariant D. However, if the approximation does not converge after MAX_LOOP_LIMIT of steps, the functions directly return them instead of reverting, causing inaccurate results being used by the contract.

As commented in the Curve implementation, the convergence typically occurs in 4 rounds or less. If the convergence doesn't happen after 256 rounds, the pool is likely in a corrupted state, where the admin should pause the trades to prevent users from losing funds accidentally or anyone exploiting such edge cases to profit. The likelihood of this happening is low, yet the severity is high. Thus I considered it as a medium-risk vulnerability.

Proof of Concept

Referenced code: HybridPool.sol#L347-L364 HybridPool.sol#L373-L387

Curve implementation of _get_D

Recommended Mitigation Steps

Consider reverting the transaction if convergence does not happen after MAX_LOOP_LIMIT rounds of approximation.

Handle

broccoli

Vulnerability details

Impact

The _depositToBentoBox and _depositFromUserToBentoBox allow users to provide ETH to the router, which is later deposited to the bento contract for swapping other assets or providing liquidity. However, in these two functions, the input parameter does not represent the actual amount of ETH to deposit, and users have to calculate the actual amount and send it to the router, causing a back-run vulnerability if there are ETH left after the operation.

Proof of Concept

1. A user wants to swap ETH to DAI. He calls exactInputSingleWithNativeToken on the router with the corresponding parameters and params.amountIn being 10. Before calling the function, he calculates bento.toAmount(wETH, 10, true) = 15 and thus send 15 ETH to the router.
2. However, at the time when his transaction is executed, bento.toAmount(wETH, amount, true) becomes to 14, which could happen if someone calls harvest on bento to update the elastic value of the wETH token.
3. As a result, only 14 ETH is transferred to the pool, and 1 ETH is left in the router. Anyone could back-run the user's transaction to retrieve the remaining 1 ETH from the router by calling the refundETH function.

Referenced code: TridentRouter.sol#L318-L351

Recommended Mitigation Steps

Directly push the remaining ETH to the sender to prevent any ETH left in the router.