Juicebox Buyback Delegate - sces60107'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#L231-L232

Vulnerability details

Impact

When calling pool.swap() in _swap(), amountSpecified is used to set the exact amount of the input token. And in uniswapV3SwapCallback(), the amount it sends to the pool is _amountToSend. Usually, _amountToSend equals amountSpecified. But it is possible that _amountToSend is less than amountSpecified, then the rest ETH would be stuck in JBXBuybackDelegate.

Proof of Concept

amountSpecified is used to set the exact amount of the input token in _swap() https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L266

    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) {
            // Swap succeded, take note of the amount of projectToken received (negative as it is an exact input)
            _amountReceived = uint256(-(_projectTokenIsZero ? amount0 : amount1));
        } catch {
            // implies _amountReceived = 0 -> will later mint when back in didPay
            return _amountReceived;
        }

        …
    }

And _amountToSend is the actual amount that is sent to the pool. https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L231-L232

    function uniswapV3SwapCallback(int256 amount0Delta, int256 amount1Delta, bytes calldata data) external override {
        …

        uint256 _amountToSend = uint256(_projectTokenIsZero ? amount1Delta : amount0Delta);

        …

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

But _amountToSend is not always the same as amountSpecified. In the following code snippet, we can find out that _amountToSend is actually amountSpecified - state.amountSpecifiedRemaining. state.amountSpecifiedRemaining is usually zero. But if the price limit is reached, state.amountSpecifiedRemaining could be not zero. https://github.com/Uniswap/v3-core/blob/main/contracts/UniswapV3Pool.sol#L769

    function swap(
        address recipient,
        bool zeroForOne,
        int256 amountSpecified,
        uint160 sqrtPriceLimitX96,
        bytes calldata data
    ) external override noDelegateCall returns (int256 amount0, int256 amount1) {
        …

        // continue swapping as long as we haven't used the entire input/output and haven't reached the price limit
        while (state.amountSpecifiedRemaining != 0 && state.sqrtPriceX96 != sqrtPriceLimitX96) {
            …

        (amount0, amount1) = zeroForOne == exactInput
            ? (amountSpecified - state.amountSpecifiedRemaining, state.amountCalculated)
            : (state.amountCalculated, amountSpecified - state.amountSpecifiedRemaining);

        // do the transfers and collect payment
        if (zeroForOne) {
            if (amount1 < 0) TransferHelper.safeTransfer(token1, recipient, uint256(-amount1));

            uint256 balance0Before = balance0();
            IUniswapV3SwapCallback(msg.sender).uniswapV3SwapCallback(amount0, amount1, data);
            require(balance0Before.add(uint256(amount0)) <= balance0(), 'IIA');
        } else {
            if (amount0 < 0) TransferHelper.safeTransfer(token0, recipient, uint256(-amount0));

            uint256 balance1Before = balance1();
            IUniswapV3SwapCallback(msg.sender).uniswapV3SwapCallback(amount0, amount1, data);
            require(balance1Before.add(uint256(amount1)) <= balance1(), 'IIA');
        }

        …

Thus, _amountToSend could be less than amountSpecified. And the surplus ETH is stuck in the contract.

Tools Used

Manual Review

Recommended Mitigation Steps

Refund the surplus ETH. Or the surplus ETH should be able to be withdrawn by the owner.

Assessed type

Uniswap

Summary

I list 1 low-critical finding:

• (Low) Anyone can call payParams() to set the mutex.

(Low) Anyone can call payParams() to set the mutex.

Impact

Anyone can call payParams() to set the mutex. There is no check on msg.sender. The intended path is calling payParams() first then calling didPay() if needed. Usually, this issue won’t harm the protocol in the intended usage.

Proof of Concept

There is no check on msg.sender. Anyone can set the mutex https://github.com/code-423n4/2023-05-juicebox/blob/main/juice-buyback/contracts/JBXBuybackDelegate.sol#L144

    function payParams(JBPayParamsData calldata _data)
        external
        override
        returns (uint256 weight, string memory memo, JBPayDelegateAllocation[] memory delegateAllocations)
    {
        …
        if (_tokenCount < _quote - (_quote * _slippage / SLIPPAGE_DENOMINATOR)) {
            // Pass the quote and reserve rate via a mutex
            mintedAmount = _tokenCount;
            reservedRate = _data.reservedRate;

           …
    }

Recommended Mitigation Steps

Add a check to ensure that msg.sender == address(jbxTerminal)