Missing deadline checks allow pending transactions to be maliciously executed

[code423n4]

Lines of code

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

Vulnerability details

Impact

JBXBuybackDelegate.sol#L258-L326

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;
    }

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

    // Send the non-reserved token to the beneficiary (if any / reserved rate is not max)
    if (_nonReservedToken != 0) projectToken.transfer(_data.beneficiary, _nonReservedToken);

    // If there are reserved token, add them to the reserve
    if (_reservedToken != 0) {
        IJBController controller = IJBController(jbxTerminal.directory().controllerOf(_data.projectId));

        // 1) Burn all the reserved token, which are in this address -> result: 0 here, 0 in reserve
        controller.burnTokensOf({
            _holder: address(this),
            _projectId: _data.projectId,
            _tokenCount: _reservedToken,
            _memo: "",
            _preferClaimedTokens: true
        });

        // 2) Mint the reserved token with this address as beneficiary -> result: _amountReceived-reserved here, reservedToken in reserve
        controller.mintTokensOf({
            _projectId: _data.projectId,
            _tokenCount: _amountReceived,
            _beneficiary: address(this),
            _memo: _data.memo,
            _preferClaimedTokens: false,
            _useReservedRate: true
        });

        // 3) Burn the non-reserve token which are now left in this address (can be 0) -> result: 0 here, reservedToken in reserve
        uint256 _nonReservedTokenInContract = _amountReceived - _reservedToken;

        if (_nonReservedTokenInContract != 0) {
            controller.burnTokensOf({
                _holder: address(this),
                _projectId: _data.projectId,
                _tokenCount: _nonReservedTokenInContract,
                _memo: "",
                _preferClaimedTokens: false
            });
        }
    }

    emit JBXBuybackDelegate_Swap(_data.projectId, _data.amount.value, _amountReceived);
}

Impact

The JBXBuybackDelegate._swap() does not allow project owners to submit a deadline when swapping WETH to project tokens.

Transaction can be pending in the mempool for a long time and without deadline check, the transaction can be executed a long time after the user submits the transaction. By the time user's transaction is executed, the swap could have been done at a sub-optimal price, resulting in lesser project tokens minted then intended. This directly conflicts with juicebox buyback protocol functionality of maximizing project token received by contributors/beneficiaries.

Proof of Concept

Consider the following scenario where Alice has a project with a stable coin known as Token A.

1. Alice wants to pay 2000 token A to Bob. She signs the transaction calling JBXBuybackDelegate.didPay() with slippage of 1% and passes in a value of 1 ETH.
   
2. The transaction is submitted to the mempool, however, Alice chose a transaction fee that is too low for validators to be interested in including her transaction in a block. The transaction stays pending in the mempool for extended periods, which could be hours, days, weeks, or even longer.
   
3. When the average gas fee dropped far enough for Alice's transaction to become interesting again for validators to include it, her swap will be executed. In the meantime, the price of ETH could have drastically decreased. Bob will still at least get minted 1980 Token A due to slippage, but the Token A value of that output might be significantly lower. She has unknowingly caused a loss to the contributor/beneficiary.

An even worse way this issue can be maliciously exploited is through MEV:

1. The _swap transaction is still pending in the mempool. Average fees are still too high for validators to be interested in it. The price of Token A has gone up significantly since the transaction was signed, meaning Bob would be minted a lot more Token A when the swap is executed. But that also means that Alice's minOutput value is outdated and would allow for significant slippage.
   
2. A MEV bot detects the pending transaction. Since the outdated slippage now allows for high slippage, the bot sandwiches Alice, resulting in significant profit for the bot and significant loss for Bob.

Tools Used

Manual Analysis

Recommendation

Intoduce a deadline parameter to the functions _swap for This can be in the form of a modifier such as

modifier ensure(uint deadline) {
    require(deadline >= block.timestamp, 'UniswapV3Router: EXPIRED');
    _;
}

Assessed type

Timing

[c4-pre-sort]

dmvt marked the issue as duplicate of #6

[dmvt]

Invalid. See comment on #6 regarding why this is not really an issue.

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid