`HybridPool._updateReserves` Wrong implementation

[code423n4]

Handle

WatchPug

Vulnerability details

https://github.com/sushiswap/trident/blob/9f949d9bddf7f12775e0c6ae641e9305c4762ea2/contracts/pool/HybridPool.sol#L277-L288

HybridPool._updateReserves() is supposed to update the reserves to the latest bento share amounts. Instead, it uses underlying token amounts.

In the _getReserves() function, reserves are treated as bento share amounts and get converted to underlying token amounts again.

When the price per share is smaller or larger than 1, the result of _getReserves() can be different from the actual underlying token amounts by a lot.

function _updateReserves() internal {
    (uint256 _reserve0, uint256 _reserve1) = _balance();
    require(_reserve0 < type(uint128).max && _reserve1 < type(uint128).max, "OVERFLOW");
    reserve0 = uint128(_reserve0);
    reserve1 = uint128(_reserve1);
    emit Sync(_reserve0, _reserve1);
}

function _balance() internal view returns (uint256 balance0, uint256 balance1) {
    balance0 = bento.toAmount(token0, bento.balanceOf(token0, address(this)), false);
    balance1 = bento.toAmount(token1, bento.balanceOf(token1, address(this)), false);
}

Impact

Multiple essential functions in the HybridPool contract is assuming the reserves are stored as shares instead of underlying token amounts, which makes this issue impacts many functions, the impact of the swap() function allows the stolen of a large portion of the funds in the pool.

Other functions that are malfunctioned because of this issue include:

• mint()
• flashSwap()
• burnSingle()
• getVirtualPrice()
• getAmountOut()

Proof of Concept

HybridPool.swap is using an unchecked calculation for amountIn: amountIn = balance0 - _reserve0;, when _reserve0 > balance0, it underflows and become an extremly large number.

amountOut will then be extremely large, near the outToken balance amount.

As a result, the attacker can steal almost all of the outToken in the pool.

https://github.com/sushiswap/trident/blob/9f949d9bddf7f12775e0c6ae641e9305c4762ea2/contracts/pool/HybridPool.sol#L179-L202

function swap(bytes calldata data) public override lock returns (uint256 amountOut) {
    (address tokenIn, address recipient, bool unwrapBento) = abi.decode(data, (address, address, bool));
    (uint256 _reserve0, uint256 _reserve1, uint256 balance0, uint256 balance1) = _getReservesAndBalances();
    uint256 amountIn;
    address tokenOut;

    if (tokenIn == token0) {
        tokenOut = token1;
        unchecked {
            amountIn = balance0 - _reserve0;
        }
        amountOut = _getAmountOut(amountIn, _reserve0, _reserve1, true);
    } else {
        require(tokenIn == token1, "INVALID_INPUT_TOKEN");
        tokenOut = token0;
        unchecked {
            amountIn = balance1 - _reserve1;
        }
        amountOut = _getAmountOut(amountIn, _reserve0, _reserve1, false);
    }
    _transfer(tokenOut, amountOut, recipient, unwrapBento);
    _updateReserves();
    emit Swap(recipient, tokenIn, tokenOut, amountIn, amountOut);
}

Recommendation

Change to:

function _updateReserves() internal {
    uint256 _reserve0 = bento.balanceOf(token0, address(this));
    uint256 _reserve1 = bento.balanceOf(token1, address(this));
    require(_reserve0 < type(uint128).max && _reserve1 < type(uint128).max, "OVERFLOW");
    reserve0 = uint128(_reserve0);
    reserve1 = uint128(_reserve1);
    emit Sync(_reserve0, _reserve1);
}

[itsmetechjay]

Withdrawn by warden. Per WatchPug: "I mistakenly submitted some issues of sushi to wild credit while having two tabs open at the same time."