Lines of code
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L168&gt;
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L154&gt;

Vulnerability details

Impact

The protocol has recognized the need to track buyers ETH in order to refund unused ETH by implementing the _returnDust function and setupExecution modifier. The implementation creates an attack vector that allows the seller to steal the unused ETH.

Refunding is promised and implemented by the protocol, therefor it is probable that buyers will send unused ETH.

Proof of Concept

The bug exists in such that a seller can re-enter the contract once getting the payment and transfer all the ETH balance of the exchange contract. Because the contract is in a state that funds have not yet been refunded to the buyer, the seller will receive them instead.

Consider the following:

1. Alice (buyer) executes an order from Bob (seller) by sending 2 ETH for a 1.5 ETH item. (expects to get 0.5 ETH back)
2. Bob receives the funds and calls bulkExecute with an empty executions array and 1 WEI.
3. Bob receives (0.5 ETH + 1 WEI) as a refund in _returnDust (contracts balance at this point).
4. Bob returns execution to Alice order.
5. Alice expects to get refunded 0.5 ETH but gets 0.

Technical Explanation:

The protocol tracks the ETH usage of the buyer in the modifier setupExecution by capturing the msg.value of the call to execute or bulkExecute in Exchange.sol.

setupExecution:
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L40&gt;

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

execute:
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L154&gt;

    function execute(Input calldata sell, Input calldata buy)
        external
        payable
        whenOpen
        setupExecution
    {
        _execute(sell, buy);
        _returnDust();
    }

bulkExecute:
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L168&gt;

    function bulkExecute(Execution[] calldata executions)
        external
        payable
        whenOpen
        setupExecution

Before the buyers (Alice) 1.5 ETH funds are sent to the seller for payment, the _remainingETH is reduced by the price of the order. After reduction, the unused ETH amount will be updated in _remainingETH (0.5 ETH)

_executeFundsTransfer:
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L565&gt;

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

The seller (Bob) receives the funds (1.5 ETH) for the order in the transferTo function which is called to send the native 1.5 ETH to the seller (Bob):

transferTo:
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L618&gt;

    function _transferTo(
        address paymentToken,
        address from,
        address to,
        uint256 amount
    ) internal {
        if (amount == 0) {
            return;
        }

        if (paymentToken == address(0)) {
            /* Transfer funds in ETH. */
            require(to != address(0), "Transfer to zero address");
            (bool success,) = payable(to).call{value: amount}("");
            require(success, "ETH transfer failed");

At this point the control flow is transferred to the seller (Bob) and the seller can re-enter the Exchange contract and call bulkExecute with an empty executions array and 1 WEI. At this state, the Exchange contract holds the unused ETH of the buyer (Alice) in its balance (0.5 ETH).

Calling bulkExecute with an empty array and 1 WEI will not call _execute because the loop will not run and execution will not be reverted by the reentrancyGuard modifier.
1 WEI is used so _returnDust at the end of bulkExecute will send the seller (Bob) the balance of the contract. In this case, the amount is equal to the unused ETH of the buyer (0.5 ETH) + 1 WEI.

bulkExecute:
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L168&gt;

    function bulkExecute(Execution[] calldata executions)
        external
        payable
        whenOpen
        setupExecution
    {
        /*
        REFERENCE
        uint256 executionsLength = executions.length;
        for (uint8 i=0; i < executionsLength; i++) {
            bytes memory data = abi.encodeWithSelector(this._execute.selector, executions[i].sell, executions[i].buy);
            (bool success,) = address(this).delegatecall(data);
        }
        _returnDust(remainingETH);
        */
        uint256 executionsLength = executions.length;
        for (uint8 i = 0; i < executionsLength; i++) {
            assembly {
                let memPointer := mload(0x40)

                let order_location := calldataload(add(executions.offset, mul(i, 0x20)))
                let order_pointer := add(executions.offset, order_location)

                let size
                switch eq(add(i, 0x01), executionsLength)
                case 1 {
                    size := sub(calldatasize(), order_pointer)
                }
                default {
                    let next_order_location := calldataload(add(executions.offset, mul(add(i, 0x01), 0x20)))
                    let next_order_pointer := add(executions.offset, next_order_location)
                    size := sub(next_order_pointer, order_pointer)
                }

                mstore(memPointer, 0xe04d94ae00000000000000000000000000000000000000000000000000000000) // _execute
                calldatacopy(add(0x04, memPointer), order_pointer, size)
                // must be put in separate transaction to bypass failed executions
                // must be put in delegatecall to maintain the authorization from the caller
                let result := delegatecall(gas(), address(), memPointer, add(size, 0x04), 0, 0)
            }
        }
        _returnDust();
    }

_returnDust:
<https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L212&gt;

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

After the seller (Bob) will receive the unused funds (0.5 ETH + 1 WEI), they can return true indicating that they received the order funds (1.5 ETH) and the rest of the execute function will continue to run.

After _execute finished running. The _returnDust function is called, _remainingETH is 0 because the state variable was updated by Bob to 0 after execution (through setupExecution).

Results:

1. The buyer (Alice) losses their unused funds (0.5 ETH).
2. Alice received Bobs unused funds (0.5 ETH) and price for item (1.5 ETH)

Tools Used

VS Code

Recommended Mitigation Steps

1. Add a reentrancy guard on bulkExecute and execute
2. in _returnDust - refund the _remainingETH and not entire balance
3. Revert if no _execute was called (would force the seller to successfully execute at-least one execution)

---

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

All reactions