ETH CAN GET LOCKED IN THE CONTRACT DURING THE EXECUTION OF _swap() FUNCTION

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L266
https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L225-L232

Vulnerability details

Impact

In the JBXBuybackDelegate delegate contract, if the swap option is selected after comparing the quote, the JBXBuybackDelegate._swap() function will swap the _data.amount.value amount of ETH in the following pool.swap() call.

    try pool.swap({
        recipient: address(this),
        zeroForOne: !_projectTokenIsZero,
        amountSpecified: int256(_data.amount.value),
        sqrtPriceLimitX96: _projectTokenIsZero ? TickMath.MAX_SQRT_RATIO - 1 : TickMath.MIN_SQRT_RATIO + 1,
        data: abi.encode(_minimumReceivedFromSwap)
    }) 

Once this call is made the Uniswap V3 Pool will call the JBXBuybackDelegate.uniswapV3SwapCallback() call back function. uniswapV3SwapCallback() function will convert the actual _amountToSend amount of ETH to WETH and then transfer that amount to the Uniswap V3 Pool.

The _amountToSend is calculated using the Uniswap V3 Pool exchange rate of the pool at the time of the execution of the transaction. Hence even though the full amount of _data.amount.value was expected to be swapped into project tokens, only _amountToSend amount of WETH will be swapped.

Based on the value of _amountToSend, the following scenarios can occur.

1. _amountToSend > _data.amount.value

The transaction will revert since there is not enough ETH in the contract to be converted into WETH and transferred into the Uniswap V3 Pool.

2. _amountToSend < _data.amount.value

The _data.amount.value - _amountToSend will get stucked in the contract. There is no withdrawal mechanism in the contract to withdraw this locked amount of Eth. Even though for a single transaction this value will be very low, once the protocol starts functioning and multiple swap transactions are executed, these smaller values can addup and result in a considerable amount of ETH locked in the contract.

This could further allow an attacker to swap _data.amount.value amount by sending in comparatively lesser amount of ETH since the remainder will be fulfilled by the locked ETH, given there is sufficient amount of ETH locked due to previous swap transactions.

Proof of Concept

    function _swap(JBDidPayData calldata _data, uint256 _minimumReceivedFromSwap, uint256 _reservedRate)
        internal
        returns (uint256 _amountReceived)
    {
        // Pass the token and min amount to receive as extra data
        try pool.swap({
            recipient: address(this),
            zeroForOne: !_projectTokenIsZero,
            amountSpecified: int256(_data.amount.value),
            sqrtPriceLimitX96: _projectTokenIsZero ? TickMath.MAX_SQRT_RATIO - 1 : TickMath.MIN_SQRT_RATIO + 1,
            data: abi.encode(_minimumReceivedFromSwap)
        }) returns (int256 amount0, int256 amount1) {
        ...
   }

https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L266

    function uniswapV3SwapCallback(int256 amount0Delta, int256 amount1Delta, bytes calldata data) external override {
        // Check if this is really a callback
        if (msg.sender != address(pool)) revert JuiceBuyback_Unauthorized();

        // Unpack the data
        (uint256 _minimumAmountReceived) = abi.decode(data, (uint256));

        // Assign 0 and 1 accordingly
        uint256 _amountReceived = uint256(-(_projectTokenIsZero ? amount0Delta : amount1Delta));
        uint256 _amountToSend = uint256(_projectTokenIsZero ? amount1Delta : amount0Delta);

        // Revert if slippage is too high
        if (_amountReceived < _minimumAmountReceived) revert JuiceBuyback_MaximumSlippage();

        // Wrap and transfer the weth to the pool
        weth.deposit{value: _amountToSend}();
        weth.transfer(address(pool), _amountToSend);
    }

https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L225-L232

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

It is recommended to implement a withdrawal function in the JBXBuybackDelegate contract to release the _data.amount.value - _amountToSend amount of ETH locked, per swap transaction. This locked amount can increase as more swap transactions are executed in the contract.

This recommended withdrawal function should only be called by the admin of the protocol. And the admin should be able to transfer this locked amount of ETH in to the protocol reserve.

Assessed type

ETH-Transfer

[c4-pre-sort]

dmvt marked the issue as duplicate of #42

[c4-judge]

dmvt marked the issue as satisfactory