Juicebox Buyback Delegate - BLACK-PANDA-REACH's results

Lines of code

https://github.com/R-E-A-C-H/blackpanda-juicebox/blob/9a36e5c8d0588f0f262a0cd1c08e34b2184d8f4d/juice-buyback/contracts/JBXBuybackDelegate.sol#L263-L275

Vulnerability details

Vulnerability details

In case that the project JBToken address is bigger than WETH address, _projectTokenIsZero is set to false. The test cases of buyback delegate only cover the situation, where the JBToken is lower than WETH.

    constructor(
        IERC20 _projectToken,
        IWETH9 _weth,
        IUniswapV3Pool _pool,
        IJBPayoutRedemptionPaymentTerminal3_1 _jbxTerminal
    ) {
        projectToken = _projectToken;
        pool = _pool;
        jbxTerminal = _jbxTerminal;
        _projectTokenIsZero = address(_projectToken) < address(_weth); // <======= sets tokens ordering for UniswapV3

In uniswap v3 pool swapping, whenever user provides more ETH than what ProjectToken is worth of, then the remaining ETH will be send back to swap() caller, in this case, JBXBuybackDelegate. Since this ETH are not sent back to original caller i.e., payer, its stuck inside JBXBuybackDelegate contract.

Impact

Users are loosing Uniswap swap leftovers

Proof of Concept

Unfortunately this test is hard to setup with test cases provided by JuiceBox team, due to specific requirements concerning project setup and block height. We came up with the test, to verify the case in which the non-WETH token is considered as project token and is bigger than WETH. Please create a new file in test folder and run forge test to test it:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";

interface ISwapRouter {
    function refundETH() external payable;
}
interface IPool {
    function slot0()
        external
        view
        returns (
            uint160 sqrtPriceX96,
            int24 tick,
            uint16 observationIndex,
            uint16 observationCardinality,
            uint16 observationCardinalityNext,
            uint8 feeProtocol,
            bool unlocked
        );
    function swap(
        address recipient,
        bool zeroForOne,
        int256 amountSpecified,
        uint160 sqrtPriceLimitX96,
        bytes calldata data
    ) external returns (int256 amount0, int256 amount1);
    function token0() external view returns(address);
    function token1() external view returns(address);
}

interface IERC20 {
    function balanceOf(address) external view returns (uint256);
}

interface IWETH9 {
    function approve(address guy, uint256 wad) external returns (bool);
    function deposit() external payable;
    function transfer(address, uint256) external;
    function balanceOf(address) external view returns (uint256);
}

contract Swapper {
    ISwapRouter router =
        ISwapRouter(0xE592427A0AEce92De3Edee1F18E0157C05861564);
    // IPool pool = IPool(0x88e6A0c2dDD26FEEb64F039a2c41296FcB3f5640);
    IPool pool = IPool(0x7379e81228514a1D2a6Cf7559203998E20598346);
    
    // IERC20 projectToken = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);
    // IWETH9 weth = IWETH9(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);    
    IERC20 projectToken = IERC20(0xFe2e637202056d30016725477c5da089Ab0A043A);
    IWETH9 weth = IWETH9(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);    

    bool _projectTokenIsZero = address(projectToken) < address(weth);

    uint160 internal constant MIN_SQRT_RATIO = 4295128739;
    uint160 internal constant MAX_SQRT_RATIO = 1461446703485210103287273052203988822378723970342;

    function check_swap(uint amountIn, uint minOutput) external payable returns(int256, int256) {
        (int256 amount0, int256 amount1) = pool.swap({
            recipient: address(this),
            zeroForOne: !_projectTokenIsZero,
            amountSpecified: int256(amountIn),
            sqrtPriceLimitX96: _projectTokenIsZero ? MAX_SQRT_RATIO - 1 : MIN_SQRT_RATIO + 1,
            data: abi.encode(minOutput)
        });

        return (amount0, amount1);
    }

    function uniswapV3SwapCallback(int256 amount0Delta, int256 amount1Delta, bytes calldata data) external {
        // Check if this is really a callback
        require(msg.sender == address(pool), "Not Authorized");

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

        // emit log_named_decimal_uint("Amount received: ", _amountReceived, 18);
        // emit log_named_decimal_uint("Amount to send: ", _amountToSend, 18);

        // Revert if slippage is too high
        require(_amountReceived >= _minimumAmountReceived, "min amount");

        // Wrap and transfer the weth to the pool
        weth.deposit{value: _amountToSend}(); //@audit-issue If there is not enough liquidity in the pool then _amountToSend will be < ETH sent to this address. In such case, remaining ETHs not refunded to the caller. Its get stuck in the contract as there is no way to recover this ETH.
        weth.transfer(address(pool), _amountToSend);
    }
    receive() external payable {}
}
contract UniswapV3ETHRefundExploitTest is Test {
    // ISwapRouter router =
    //     ISwapRouter(0xE592427A0AEce92De3Edee1F18E0157C05861564);
    // // IPool pool = IPool(0x88e6A0c2dDD26FEEb64F039a2c41296FcB3f5640);
    IPool pool = IPool(0x7379e81228514a1D2a6Cf7559203998E20598346);
    
    // // IERC20 projectToken = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);
    // // IWETH9 weth = IWETH9(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);    
    IERC20 projectToken = IERC20(0xFe2e637202056d30016725477c5da089Ab0A043A);
    IWETH9 weth = IWETH9(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);    

    // bool _projectTokenIsZero = address(projectToken) < address(weth);

    // uint160 internal constant MIN_SQRT_RATIO = 4295128739;
    // uint160 internal constant MAX_SQRT_RATIO = 1461446703485210103287273052203988822378723970342;

    function setUp() public {
        vm.label(address(projectToken), "ProjectToken");
        vm.label(address(weth), "WETH");
    }
    function testExploit() public {
        vm.createSelectFork("https://rpc.ankr.com/eth");

        Swapper swapper = new Swapper();

        vm.label(address(swapper), "swapper");

        address user = makeAddr("user");
        uint256 amountIn = 1000_000 ether;
        uint256 minOutput = 0;

        vm.label(user, "user");
        vm.deal(user, amountIn);

        emit log_named_decimal_uint("B4: Balance of Pool's WETH:", weth.balanceOf(address(pool)), 18);
        emit log_named_decimal_uint("B4: Balance of Pool's Project Token:", projectToken.balanceOf(address(pool)), 18);

        emit log_named_decimal_uint("Before: Balance of ETH (swapper):", address(swapper).balance, 18);
        emit log_named_decimal_uint("Before: Balance of WETH (swapper):", weth.balanceOf(address(swapper)), 18);
        emit log_named_decimal_uint("Before: Balance of Project Token (swapper):", projectToken.balanceOf(address(swapper)), 18);

        emit log_named_decimal_uint("Before: Balance of ETH (user):", user.balance, 18);
        emit log_named_decimal_uint("Before: Balance of WETH (user):", weth.balanceOf(user), 18);
        emit log_named_decimal_uint("Before: Balance of Project Token (user):", projectToken.balanceOf(user), 18);

        vm.prank(user);
        (int256 amount0, int256 amount1) = swapper.check_swap{value: amountIn}(amountIn, minOutput);

        console2.log("Amount 0:", amount0);
        console2.log("Amount 1:", amount1);

        // router.refundETH();

        emit log_named_decimal_uint("After: Balance of ETH (swapper):", address(swapper).balance, 18);
        emit log_named_decimal_uint("After: Balance of WETH (swapper):", weth.balanceOf(address(swapper)), 18);
        emit log_named_decimal_uint("After: Balance of Project Token (swapper):", projectToken.balanceOf(address(swapper)), 18);

        emit log_named_decimal_uint("After: Balance of ETH (user):", user.balance, 18);
        emit log_named_decimal_uint("After: Balance of WETH (user):", weth.balanceOf(user), 18);
        emit log_named_decimal_uint("After: Balance of Project Token (user):", projectToken.balanceOf(user), 18);

        emit log_named_decimal_uint("Ar: Balance of Pool's WETH:", weth.balanceOf(address(pool)), 18);
        emit log_named_decimal_uint("Ar: Balance of Pool's Project Token:", projectToken.balanceOf(address(pool)), 18);
    }
}

Tools Used

Manual analysis

Recommended Mitigation Steps

We recommend either returning the funds to the caller, or minting the leftover amount to them.

Assessed type

Uniswap

[L-01]: Unnecessary computation when _data.preferClaimedTokens=false

Code snippet

https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/JBXBuybackDelegate.sol#L195-L206

Line number from 195-197 always getting executed, even when those variables are not needed in subsequent call when _data.preferClaimedTokens=false. If line number 195-197 moved inside if block at line 200, we can save some gas for the call, by avoiding computation when _data.preferClaimedTokens = false

diff --git a/JBXBuybackDelegate_orig.sol b/JBXBuybackDelegate_mod.sol
index 0ee751b..44717ec 100644
--- a/JBXBuybackDelegate_orig.sol
+++ b/JBXBuybackDelegate_mod.sol
@@ -192,12 +192,12 @@ contract JBXBuybackDelegate is IJBFundingCycleDataSource, IJBPayDelegate, IUnisw
         uint256 _reservedRate = reservedRate;
         reservedRate = 1;
 
-        // The minimum amount of token received if swapping
-        (,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));
-        uint256 _minimumReceivedFromSwap = _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR);
-
         // Pick the appropriate pathway (swap vs mint), use mint if non-claimed prefered
         if (_data.preferClaimedTokens) {
+            // The minimum amount of token received if swapping
+            (,, uint256 _quote, uint256 _slippage) = abi.decode(_data.metadata, (bytes32, bytes32, uint256, uint256));
+            uint256 _minimumReceivedFromSwap = _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR);
+
             // Try swapping
             uint256 _amountReceived = _swap(_data, _minimumReceivedFromSwap, _reservedRate);

[L-02] No sanity checks in the constructor can lead to issues or redeployments

Code snippet

https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/JBXBuybackDelegate.sol#L118-L129

Vulnerability description

In the constructor no checks are done for the addresses the contract is setting (like the project token, the pool...). Even if the deployer is trusted, it is normal to have typos or man-made mistakes and those mistakes here could mean a bricked contract or the need to redeploy it.

The constructor should perform some validations to the addresses sent or at least, check they are not address zero.

Mitigation

Include some argument validation in the constructor.

[L-03] Inconsistency between documentation and code around ERC20 compliance

Code snippet

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

Description

In the Juicebox documentation it states:

By default, the protocol allocates tokens to recipients using an internal accounting mechanism in JBTokenStore. These are fungible but do not conform to the ERC-20 standard, and as such cannot be composed with ecosystem ERC-20/ERC-721 marketplaces like AMMs and Opensea. Their balances can be used for voting on various platforms. Projects can issue their own ERC-20 token directly from the protocol to use as its token. Projects can also bring their own token as long as it conforms to the IJBToken interface and uses 18 decimal fixed point accounting. This makes it possible to use ERC-1155's or custom tokens.

This suggest that the tokens used by the project do not need to conform to ERC20 standard. In JBXBuybackDelegate however projectToken is casted as IERC20:

constructor(
    IERC20 _projectToken,

[L-04] Address of tokens in pool can be different than the ones passed to the constructor.

Code snippet

https://github.com/code-423n4/2023-05-juicebox/blob/9d0458282511ff269b3b35b5b082b56d5cc08663/juice-buyback/contracts/JBXBuybackDelegate.sol#L118-L129

Vulnerability description

In constructor, there is no implementation to ensure that the projectToken and weth addresses are the same as the addresses of the tokens swapped in pool. This can lead lack of synchronisation between pool and tokens, forcing the contract to be redeployed.

Mitigation

Get token addresses from a trusted source or verify token addresses passed in constructor against the ones stored in a pool contract.

[L-05] Unnecessary calculation of _nonReservedTokenInContract:

Code Snippet

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

Description

The following line is unnecesary to calculate because it's already been calculated before so we can get the older variable.

uint256 _nonReservedTokenInContract = _amountReceived - _reservedToken;

Always _nonReservedTokenInContract will be equal to _nonReservedToken, calculated here:

// The amount to send to the beneficiary
uint256 _nonReservedToken = PRBMath.mulDiv(
    _amountReceived, JBConstants.MAX_RESERVED_RATE - _reservedRate, JBConstants.MAX_RESERVED_RATE
);

// The amount to add to the reserved token
uint256 _reservedToken = _amountReceived - _nonReservedToken;