Lines of code

Vulnerability details

Description

remainingETH is an important state variable in Exchange.sol, which keeps track of how many ETH have yet to be used as payment from the current msg.value.
The setupExecution modifier sets the value before and after execution:

modifier setupExecution() {
    remainingETH = msg.value;
    isInternal = true;
    _;
    remainingETH = 0;
    isInternal = false;
}

Payments are deducted from remainingETH:

function _executeFundsTransfer(
    address seller,
    address buyer,
    address paymentToken,
    Fee[] calldata fees,
    uint256 price
) internal {
    if (msg.sender == buyer && paymentToken == address(0)) {
        require(remainingETH >= price);
        remainingETH -= price;
    }
    /* Take fee. */
    uint256 receiveAmount = _transferFees(fees, paymentToken, buyer, price);
    /* Transfer remainder to seller. */
    _transferTo(paymentToken, buyer, seller, receiveAmount);
}

Another important note is that _returnDust() is executed in the end of execute() / bulkExecute() to send back to msg.sender the remaining ETH:

function _returnDust() private {
    uint256 _remainingETH = remainingETH;
    assembly {
        if gt(_remainingETH, 0) {
            let callStatus := call(
                gas(),
                caller(),
                selfbalance(),
                0,
                0,
                0,
                0
            )
        }
    }
}

The issue created here is that _returnDust passes execution to caller without updating remainingETH. Caller may then immediately enter _execute() and use the remainingETH sum as if it is still remaining.
Interestly this is possible despite _execute being guarded by reentrancyGuard and internalCall. reentrancyGuard is bypassed because in the outer call we are in bulkExecute(), not _execute. internalCall is bypassed because while we are in the outer bulkExecute(), the setupExecution() modifier guarantees that isInternal is set to true.
Therefore, in the latter _executeFundsTransfer remainingETH will be old instead of zero, and the require will succeed.

Impact

Attacker can spoof remainingETH and double-spend their input ETH to Exchange

Proof of Concept

The call chain would look like:

• bulkExecute()
  • _execute()
    • _executeFundsTransfer()
  • _returnDust()
    • caller.call()
      • _execute()
        • _executeFundsTransfer()

Tools Used

Manual audit

Recommended Mitigation Steps

Use the Checks / Effects / Interactions pattern:
Before calling user in _returnDust(), update remainingETH to 0.

---

The text was updated successfully, but these errors were encountered:

All reactions