Juicebox Buyback Delegate - Madalad's results

Lines of code

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

Vulnerability details

Impact

The delegate is intended to facilitate a swap from ether to projectToken, through either a Uniswap swap or just by minting. The didPay function is marked payable to accept the ether for this process, however there are no checks on msg.value. This means that it is possible to send the incorrect amount of ether with the transaction, and if too much is sent then there is no functionality to refund the excess ether back to the sender.

Should the contracts ether balance be non-zero due to this, the next user is able to steal these funds on their next request, by sending less ether than is specified in _data.amount.value, and therefore using the contracts residual ether balance as part of their payment.

Proof of Concept

• Alice calls didPay and accidentally sends 2 ether while _data.amount.value is set to 1 ether
• Her transaction executes successfully, and JBXBuybackDelegate now has a balance of 1 ether
• Bob sees this, and decides to take advantage by calling didPay with _data.amount.value set to 1 ether, but sends 0 ether with the transaction
• The transaction is successful, because when the contract attempts to send ether to pay for the projectTokens, (line 231-232 if swapping, line 348 if minting) it will use the 1 ether leftover from Alice's transaction
• Alice has lost 1 ether and Bob has effectively stolen it and converted it to projectToken

231:        weth.deposit{value: _amountToSend}();
232:        weth.transfer(address(pool), _amountToSend);

348:        jbxTerminal.addToBalanceOf{value: _data.amount.value}(
349:            _data.projectId, _data.amount.value, JBTokens.ETH, "", new bytes(0)
350:        );

Tools Used

Manual review

Recommended Mitigation Steps

Add a check in didPay on msg.value.

    function didPay(JBDidPayData calldata _data) external payable override {
        // Access control as minting is authorized to this delegate
        if (msg.sender != address(jbxTerminal)) revert JuiceBuyback_Unauthorized();

+        if (msg.value != _data.amount.value) revert();

        // Retrieve the number of token created if minting and reset the mutex (not exposed in JBDidPayData)
        uint256 _tokenCount = mintedAmount;
        mintedAmount = 1;

        // Retrieve the fc reserved rate and reset the mutex
        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) {
            // Try swapping
            uint256 _amountReceived = _swap(_data, _minimumReceivedFromSwap, _reservedRate);

            // If swap failed, mint instead, with the original weight + add to balance the token in
            if (_amountReceived == 0) _mint(_data, _tokenCount);
        } else {
            _mint(_data, _tokenCount);
        }
    }

Assessed type

Invalid Validation