Blur Exchange contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-11-non-fungible/blob/8eaeb007b81d3c89ecf6a5dc2a67eecbe94cefaf/contracts/Exchange.sol#L161 https://github.com/code-423n4/2022-11-non-fungible/blob/8eaeb007b81d3c89ecf6a5dc2a67eecbe94cefaf/contracts/Exchange.sol#L209

Vulnerability details

Impact

The internalCall modifier ensures that public functions can only be called from another public function and not directly. It is implemented like that:

modifier internalCall() {
        require(isInternal, "This function should not be called directly");
        _;
}

The modifier setupExecution sets isInternal such that the calls work within all functions that have this modifier:

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

However, at the end of execute and bulkExecute, there is a _returnDust() call, which transfers ETH to the caller and therefore gives him the possibility to reenter into the contract. Because isInternal is still true at this point, this can be used to directly call functions with an isInternal modifier (i.e., the ones that should not be directly callable).

Proof of Concept

Bob calls execute (from a smart contract) to execute an order. Within _returnDust(), some ETH is transferred back to the smart contract, which triggers his receive function. In there, he can now execute all functions with an isInternal modifier, making the modifier useless. For instance, he can directly call _execute, which should not be possible (as this would be a private function, but was marked public with the modifier such that the delegatecalls work).

Recommended Mitigation Steps

Add reentrancy guard protection to setupExecution. Note that the normal reentrancy variable (that is also used for _execute) cannot be set, otherwise this call would revert. Instead, a more advanced scheme is needed with multiple variables.