Juicebox Buyback Delegate - Udsen's results

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

Lines of code

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

Vulnerability details

Impact

In the JBXBuybackDelegate contract, the terminal token is considered to be ETH as of now (according to documentation). Hence both the _mint() and _swap() functionality uses the _data.amount.value as the ETH amount for new token minting or swapping.

In the JBXBuybackDelegate.payParams() function, the _data.amount.value is used to calculate the total number of tokens to be minted (_tokenCount).

In the JBXBuybackDelegate._swap() function, the _data.amount.value is used as the amountSpecified parameter in the pool.swap() call. Which means _data.amount.value is used as the ETH amount to be swapped for project tokens.

In the JBXBuybackDelegate contract, both _mint() and _swap() function are called in the JBXBuybackDelegate.didPay() function which is the only payable function in the contract. Which means didPay() is the only function which is capable of recieving ETH into the contract.

But inside the JBXBuybackDelegate.didPay() there is no check to make sure the user of the protocol, has actually sent in the ETH amount he has mentioned in the metadata field of _data.amount.value.

In the case _swap() is selected due to its higher quote, user can send in a less amount of ETH compared to _data.amount.value he has mentioned previously in the _data metadata field,

User can do this by calling the JBXBuybackDelegate.didPay() function call, and execute the transaction succesfully if there is left over ETH (locked ETH) in the contract which is enough to execute the transaction.

So for the transaction to succeed the locked amount of ETH in the contract should at least be equal or more than the following amount :

     lockedETH >= (_data.amount.value - msg.value) 

Same scenario applies for the _mint() function as well. Where more tokens can be minted using the locked ETH amount in the contract.

Hence a malicious user can take advantage of the locked eth in the contract by sending in a less amount of ETH as msg.value in comparison to _data.amount.value and fulfilling the remainder, using the locked ETH of the contract.

This is similar to stealing funds from other users.

Note : The ETH can locked in the contract over a period of time due to the way _swap() function is implemented. This is described in another bug report submitted by me.

Proof of Concept

    function payParams(JBPayParamsData calldata _data)
        external
        override
        returns (uint256 weight, string memory memo, JBPayDelegateAllocation[] memory delegateAllocations)
    {
        // Find the total number of tokens to mint, as a fixed point number with 18 decimals
        uint256 _tokenCount = PRBMath.mulDiv(_data.amount.value, _data.weight, 10 ** 18);

        ...
    }

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

    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

Tools Used

Manual Review and VSCode

Recommended Mitigation Steps

To mitigate the above vulnerability it is recommended to conduct the following check in the JBXBuybackDelegate.didPay() function.

require(msg.value == _data.amount.value, "Incorrect ETH amount deposited");

Hence the transaction will revert if the correct amount of ETH is not deposited as given in the _data.amount.value metadata field.

Assessed type

Other

1. PROPER natspec COMMENTING STANDARD IS NOT USED.

Proper Natspec commenting standard is not used and the comments are not clear for the developers and auditors to understand.

Observed across entire JBXBuybackDelegate smart contract.

2. Typos DETECTED IN THE natspec COMMENTS.

 * @notice The Uniswap V3 pool callback (where token transfer should `happens`)
 

Should be corrected as happen https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L212

 * @dev    Slippage `controle` is achieved here
 

Should be corrected as control https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L214

 * @notice Mint the token out, sending back the token in `in` the terminal 
 

An extra in should be removed from the comment. https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L329

 * @notice Swap the terminal token to receive the project `toke_beforeTransferTon`
 

Typo in the above comment in the highlighted section. https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L246

// 2) Mint the `reserved token` with this address as beneficiary -> result: _amountReceived-reserved here, reservedToken in reserve

Above comment should be corrected as below for better understanding.

// 2) Mint the `_amountReceived` with this address as beneficiary -> result: _amountReceived-reserved here, reservedToken in reserve

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