Blur Exchange contest - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. Function _returnDust() is called in _bulkExecute but its return value has never been checked. As a result, if the ETH refund transfer fails, it will be overlooked and the transaction _bulkExecute will complete successfully but the user will lost his ETH In the contract.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L209

Scenario: Bob created a trade contract C but forgot to include a fallback/receive function to receive eth. He buys some tokens with 10 ETH by executing function _bulkExecute and there are 9 ETH remained after the trade. However, L209 will fail to transfer the ETH back to him but the transaction will compete successfully. Bob lost his 9 ETH to the contract.

Tools Used

Remix

Recommended Mitigation Steps

Revise the function _returnDust() so that it will revert when the ETH transfer fails.

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