Blur Exchange contest - 0xDecorativePineapple's results

Lines of code

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

Vulnerability details

Impact

When an order is matched, the Buyer has the option to pay in either ETH, WETH or via the Pool contract. The Exchange smart contract implements a function _returnDust which returns the extra ETH to the user, if (s)he overpays. The function is implemented in inline assembly(YUL) and takes advantage of the call OPCODE. The result of the call function call isn't checked whether it's true or not. So, if a smart contracts calls the execute function, pays in ETH and forgets to implement a fallback function, the execute function succeedsand the remaining ETH are left in the contract and not returned to the user.

Proof of Concept

• UserA creates a smart contract and transfers funds to it, but forgets to create a fallback function
• UserA matches an order and calls the execute function of the Exchange.sol with an amountA higher than the sell amount and with address(0) as the paymentToken meaning that (s)he wants to pay in ETH.
• After the checks the remainingETH are remainingETH=startingETH-sellAmount and need to be transferred to the userA via the _returnDust function
• The low level call fails, as no fallback function is implemented but the execute function succeeds and the remaining ETH are stuck in the contract.

Tools Used

Manual Code Review

Recommended Mitigation Steps

It is recommended to check the returned value of the call opcode, callStatus whether its true or not. A simple _returnDust function is given bellow:

function _returnDust() private {
    (bool success,) = payable(msg.sender).call{value: remainingETH}("");
    require(success, "ETH transfer failed");
}

Lines of code

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

Vulnerability details

Impact

The Pool smart contract allows a user to predeposit ETH so that it can be used when a seller takes their bid. It uses an ERC1967 proxy pattern and only the exchange contract is permitted to make transfers. The smart contract inherits the OwnableUpgradeable.sol smart contract

The smart contract inherits the OwnableUpgradeable.sol but never initialize it, with a call to the internal __Ownable_init(); function. Due to the fact that the upgradable smart contract initial logic cannot be executed in the constructor, this efficiently leaves the smart contract without an owner, and so, no-one can upgrade it(https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L15)

Tools Used

Manual Code Review

Recommended Mitigation Steps

It is recommended to create an initialize() function, similar to the one that the Exchange.sol , that makes a call to the __Ownable_init(); internal function. This will make the msg.sender as the original owner, that is required in order to upgrade the smart contract.