Sushi Trident contest phase 1 - 0xsanson's results

Handle

0xsanson

Vulnerability details

Impact

The flashswap function in IndexPool.sol doesn't fulfill its function. Indeed it should transfer tokens to the users before they need to pay back, but the transfer happens at the end:

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

Recommended Mitigation Steps

Move _transfer before the callback.

Handle

0xsanson

Vulnerability details

Impact

In IndexPool.sol, the function _pow is called during the computation of the output amount when swapping.

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

It's my understanding that it should calculate the exponentiation _pow(a,n) = a**n. However a simple test shows it doesn't work: _pow(3,5) = 3, _pow(3,4) = 10**18.

Recommended Mitigation Steps

Consider documenting the function (and write some tests to check it works). If it needs to compute the exponentiation, one correct way would be (check wiki for alternatives):

function _pow(uint256 a, uint256 n) internal pure returns (uint256 output) {
    if (n == 0) {
        output = 1;
        return output;
    }
    uint256 y = 1;
    while (n > 1) {
        if (n % 2 == 0) {
            a = a*a;
            n = n/2;
        } else {
            y = a*y;
            a = a*a;
            n = (n-1)/2;
        }
    }
    output = a*y;
}

Handle

0xsanson

Vulnerability details

Impact

In HybridPool's _updateReserves() function, the reserves are calculated as:

(uint256 _reserve0, uint256 _reserve1) = _balance();

This is incorrect, since reserves are a bento-share quantity, while _balance() outputs a bento-amount quantity. This basically inflates artificially the reserves causing accounting issues.

Recommended Mitigation Steps

The correct version should be uint256 _reserve0 = __balance(token0); (the double-underscore balance returns a share quantity).

Handle

0xsanson

Vulnerability details

Impact

IndexPool doesn't collect fees for barFeeTo. Since this Pool contains also a method updateBarFee(), probably this is an unintended behavior. Also without a fee, liquidity providers would probably ditch ConstantProductPool in favor of IndexPool (using the same two tokens with equal weights), since they get all the rewards. This would constitute an issue for the ecosystem.

Recommended Mitigation Steps

Add a way to send barFees to barFeeTo, same as the other pools.

Handle

0xsanson

Vulnerability details

Impact

In HybridPool's flashSwap function there's a transfer to barFeeTo

_transfer(tokenIn, fee, barFeeTo, false);

Here fee = (amountIn * swapFee) / MAX_FEE is the total swap fee. However it should transfer out only a fraction of it (barFee/MAX_FEE) otherwise liquidity providers wouldn't get any reward.

Recommended Mitigation Steps

Calculate the appropriate fee to send to the barFeeTo.

Handle

0xsanson

Vulnerability details

Impact

In HybridPool.sol, functions _computeLiquidityFromAdjustedBalances, _getY and _getYD may finish before approximation converge, since it's limited by MAX_LOOP_LIMIT iterations. In this situation the final estimated value will still be treated as correct, even though it could be relatively inaccurate.

Recommended Mitigation Steps

Consider reverting the transactions if this doesn't occur. See https://blog.openzeppelin.com/saddle-contracts-audit/ issue [M03], with their relative fix.