Sushi Trident contest phase 1 - cmichel's results

Handle

cmichel

Vulnerability details

The IndexPool.flashSwap function calls ITridentCallee(msg.sender).tridentSwapCallback(context) before transferring the tokens to the recipient via _tranfer.

Impact

It's very important that the tokens are transferred to the caller before the callback. The use-case of the flashSwap is that the caller can already use the tokens to close a loan, trade, or use these funds in a way to get the original amountIn tokens that they need to pay.

In its current form, the flashSwap does not perform its expected behavior and is not very useful.

Recommended Mitigation Steps

Call _transfer before the ITridentCallee(msg.sender).tridentSwapCallback(context).

Handle

cmichel

Vulnerability details

The IndexPool._compute function is indented as if the if (n % 2 != 0) output = output * a; is inside the loop but there are actually not braces around it. It must be in the loop for the exponentiation by repeated squaring algorithm to work:

function _pow(uint256 a, uint256 n) internal pure returns (uint256 output) {
    output = n % 2 != 0 ? a : BASE;
    for (n /= 2; n != 0; n /= 2)
        // @audit this is not divided by 1e18 again
        a = a * a;
        // @audit this is outside of the loop but should not. And output is not divided by 1e18 again
        if (n % 2 != 0) output = output * a;
}

Furthermore, the a values are treated as floating points with 18 decimals but never normalized. Each time a = a * a is computed, its magnitude increases by 1e18. It should be a = a * a / 1e18.

This contract is basically Balancer and this function should equal Balancer's bpowi. (And _powApprox is bpowApprox, _compute is bpow. These seem to be correct.)

Impact

Note that the n parameter in _pow is an integer number that represents the integer part of the token weight division: tokenInWeight / tokenOutWeight

The AMM does not work correctly when using token weights that are not equal. All AMMs that will use an iteration of this loop in _pow will return wrong swap results, or even worse always break as the a = a * a value overflows the max uint256 type as it increases by 1e18 each time.

Recommended Mitigation Steps

1. Put braces around the for loop and include the if. "Normalize" the a and output values in _pow:

function _pow(uint256 a, uint256 n) internal pure returns (uint256 output) {
    output = n % 2 != 0 ? a : BASE;
    for (n /= 2; n != 0; n /= 2) {
        // @audit balancer uses bmul here (/ 1e18) and at output
        a = _mul(a, a);
        if (n % 2 != 0) output = _mul(output, a);
    }
}

2. Add tests that cover this loop. The token weights should be different, for example 8e18 and 1e18 such that this loop is executed 3 (n = 8e18/1e18 = 8) times. The current tests use USDT/USDC with equal weights and never execute this loop.

3. Add a backlink to the original balancer functions such that it's easier for other contributors that might not be familiar with it to know what's going on.

Handle

cmichel

Vulnerability details

The HybridPool's reserves are stored as Bento "amounts" (not Bento shares) in _updateReserves because _balance() converts the current share balance to amount balances. However, when retrieving the reserve0/1 storage fields in _getReserves, they are converted to amounts a second time.

Impact

The HybridPool returns wrong reserves which affects all minting/burning and swap functions. They all return wrong results making the pool eventually economically exploitable or leading to users receiving less tokens than they should.

POC

Imagine the current Bento amount / share price being 1.5. The pool's Bento share balance being 1000. _updateReserves will store a reserve of 1.5 * 1000 = 1500. When anyone trades using the swap function, _getReserves() is called and multiplies it by 1.5 again, leading to using a reserve of 2250 instead of 1500. A higher reserve for the output token leads to receiving more tokens as the swap output. Thus the pool lost tokens and the LPs suffer this loss.

Recommended Mitigation Steps

Make sure that the reserves are in the correct amounts.

Handle

cmichel

Vulnerability details

The IndexPool.mint function performs an unsafe cast of ratio to the uint120 type:

uint120 ratio = uint120(_div(toMint, totalSupply));

Note that toMint is chosen by the caller and when choosing toMint = 2**120 * totalSupply / BASE, the ratio variable will be 2**120 and then truncated to 0 due to the cast.

This allows an attacker to mint LP tokens for free. They just need to choose the ratio such that the amountIn = ratio * reserve / BASE variable passes the require(amountIn >= MIN_BALANCE, "MIN_BALANCE"); check. For example, when choosing ratio = 2**120 * totalSupply / BASE + 1e16, an attacker has to pay 1/100th of the current reserves but heavily inflates the LP token supply.

They can then use the inflated LP tokens they received in burn to withdraw the entire pool reserves.

POC

I created this POC that implements a hardhat test and shows how to steal the pool tokens:

https://gist.github.com/MrToph/0c8b6b5ffac0673b2f72412cf4b0b099

Impact

An attacker can inflate the LP token pool supply and mint themselves a lot of LP tokens by providing almost no tokens themselves. The entire pool tokens can be stolen.

Recommended Mitigation Steps

Even though Solidity 0.8.x is used, type casts do not throw an error. A SafeCast library must be used everywhere a typecast is done.

Handle

cmichel

Vulnerability details

The IndexPool.burn/burnSingle/swap/flashSwap functions all perform unsafe type casts to uint120 without checking if the values actually fit into 120 bits.

Impact

Unlike mint, these functions have other require statements and checks that don't seem to directly lead to exploits. The safe type casts should still be implemented for additional security.

Recommended Mitigation Steps

A SafeCast library must be used everywhere a typecast is done.

Handle

cmichel

Vulnerability details

The IndexPool.constructor function already mints INIT_POOL_SUPPLY = 100 * 1e18 = 1e20 LP tokens to the zero address.

When trying to use the pool, someone has to provide the actual initial reserve tokens in mint. On the first mint, the pool reserves are zero and the token amount required to mint is just this ratio itself: uint120 amountIn = reserve != 0 ? uint120(_mul(ratio, reserve)) : ratio;

Note that the amountIn is independent of the token which does not make much sense. This implies that all tokens must be provided in equal "raw amounts", regardless of their decimals and value.

POC

Issue 1

Imagine I want to create a DAI/WBTC pool. If I want to initialize the pool with 100$ of DAI, amountIn = ratio needs to be 100*1e18=1e20 as DAI has 18 decimals. However, I now also need to supply 1e20 of WBTC (which has 8 decimals) and I'd need to pay 1e20/1e8 * priceOfBTC, over a quadrillion dollars to match it with the 100$ of DAI.

Issue 2

Even in a pool where all tokens have the same decimals and the same value, like USDC <> USDT, it leads to issues:

• Initial minter calls mint with toMint = 1e20 which sets ratio = 1e20 * 1e18 / 1e20 = 1e18 and thus amountIn = 1e18 as well. The total supply increases to 2e20.
• Second minter needs to pay less tokens to receive the same amount of 1e18 LP tokens as the first minter. This should never be the case. toMint = 1e20 => ratio = 1e20 * 1e18 / 2e20 = 0.5e18. Then amountIn = ratio * reserve / 1e18 = 0.5*reserve = 0.5e18. They only pay half of what the first LP provider had to pay.

Impact

It's unclear why it's assumed that the pool's tokens are all in equal value - this is not a StableSwap-like pool.

Any pool that uses tokens that don't have the same value and share the same decimals cannot be used because initial liquidity cannot be provided in an economically justifiable way.

It also leads to issues where the second LP supplier has to pay less tokens to receive the exact same amount of LP tokens that the initial minter receives. They can steal from the initial LP provider by burning these tokens again.

Recommended Mitigation Steps

Do not mint the initial token supply to the zero address in the constructor.

Do it like Uniswap/Balancer and let the first liquidity provider provide arbitrary token amounts, then mint the initial pool supply. If reserve == 0, amountIn should just take the pool balances that were transferred to this account.

In case the initial mint to the zero address in the constructor was done to prevent the "Uniswap-attack" where the price of a single wei of LP token can be very high and price out LPs, send a small fraction of this initial LP supply (~1000) to the zero address after it was minted to the first supplier in mint.

Handle

cmichel

Vulnerability details

The ConstantProductPool.burnSingle function is basically a burn followed by a swap and must therefore act the same way as calling these two functions sequentially.

The token amounts to redeem (amount0, amount1) are computed on the balance (not the reserve). However, the swap amount is then computed on the reserves and not the balance. The burn function would have updated the reserve to the balances and therefore balance should be used here:

amount1 += _getAmountOut(amount0, _reserve0 - amount0, _reserve1 - amount1);

⚠️ The same issue occurs in the HybridPool.burnSingle.

Impact

For a burn, usually the reserve should equal the balance, however if any new tokens are sent to the contract and balance > reserve, this function will return slightly less swap amounts.

Recommended Mitigation Steps

Call _getAmountOut with the balances instead of the reserves: _getAmountOut(amount0, balance0 - amount0, balance1 - amount1)

Handle

cmichel

Vulnerability details

The TridentRouter.complexPath function allows splitting a trade result into several buckets and trade them in a different pool each. The distribution is defined by the params.percentagePath[i].balancePercentage values:

for (uint256 i; i < params.percentagePath.length; i++) {
    uint256 balanceShares = bento.balanceOf(params.percentagePath[i].tokenIn, address(this));
    uint256 transferShares = (balanceShares * params.percentagePath[i].balancePercentage) / uint256(10)**8;
    bento.transfer(params.percentagePath[i].tokenIn, address(this), params.percentagePath[i].pool, transferShares);
    isWhiteListed(params.percentagePath[i].pool);
    IPool(params.percentagePath[i].pool).swap(params.percentagePath[i].data);
}

However, the base value bento.balanceOf(params.percentagePath[i].tokenIn, address(this)); is recomputed after each iteration instead of caching it before the loop.

This leads to not all tokens being used even though the percentages add up to 100%.

POC

Assume I want to trade 50% DAI to WETH and the other 50% DAI to WBTC. In the first iteration, balanceShares is computed and then 50% of it is swapped in the first pool.

However, in the second iteration, balanceShares is updated again, and only 50% of the remaining (instead of the total) balance, i.e. 25%, is traded.

The final 25% are lost and can be skimmed by anyone afterwards.

Impact

Users can lose their funds using complexPath.

Recommended Mitigation Steps

Cache the balanceShares value once before the second for loop starts.

Handle

cmichel

Vulnerability details

The TridentRouter._depositToBentoBox function only uses the ETH in the contract if it's higher then the desired underlyingAmount (address(this).balance >= underlyingAmount)).

Otherwise, the ETH is ignored and the function uses WETH from the user.

Impact

Note that the underlyingAmount = bento.toAmount(wETH, amount, true) is computed from the Bento share price and it might happen that it increases from the time the transaction was submitted to the time the transaction is included in a block. In that case, it might completely ignore the sent ETH balance from the user and in addition transfer the same amount of WETH from the user.

The user can lose their ETH deposit in the contract.

Recommended Mitigation Steps

Each batch must use refundETH at the end.

Furthermore, we recommend still depositing address(this).balance ETH into Bento and if it's less than underlyingAmount use WETH only for the remaining token difference.

Handle

cmichel

Vulnerability details

The TridentHelper.withdrawFromWETH (used in TridentRouter.unwrapWETH) function performs a low-level call to WETH.withdraw(amount).

It then checks if the return data length is more or equal to 32 bytes, however WETH.withdraw returns void and has a return value of 0. Thus, the function always reverts even if success == true.

function withdrawFromWETH(uint256 amount) internal {
    // @audit WETH.withdraw returns nothing, data.length always zero. this always reverts
    require(success && data.length >= 32, "WITHDRAW_FROM_WETH_FAILED");
}

Impact

The unwrapWETH function is broken and makes all transactions revert. Batch calls to the router cannot perform any unwrapping of WETH.

Recommended Mitigation Steps

Remove the data.length >= 32 from the require and only check if success is true.

Handle

cmichel

Vulnerability details

The HybridPool.flashSwap function sends the entire trade fees fee to the barFeeTo. It should only send barFee * fee to the barFeeTo address.

Impact

LPs are not getting paid at all when this function is used. There is no incentive to provide liquidity.

Recommended Mitigation Steps

The flashSwap function should use the same fee mechanism as swap and only send barFee * fee / MAX_FEE to the barFeeTo. See _handleFee function.