code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/Executor.sol#L222-L242 https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/BasicOrderFulfiller.sol#L962-L966

Vulnerability details

Impact

The protocol doesn't check to address when transferring ETH, leading to users may lose ETH accidentally.

Proof of Concept

In _transferEth, it use call to transfer ETH to payable to address: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/Executor.sol#L222-L242

    function _transferEth(address payable to, uint256 amount) internal {
        // Ensure that the supplied amount is non-zero.
        _assertNonZeroAmount(amount);

        // Declare a variable indicating whether the call was successful or not.
        bool success;

        assembly {
            // Transfer the ETH and store if it succeeded or not.
            success := call(gas(), to, amount, 0, 0, 0, 0)
        }

        // If the call fails...
        if (!success) {
            // Revert and pass the revert reason along if one was returned.
            _revertWithReasonIfOneIsReturned();

            // Otherwise, revert with a generic error message.
            revert EtherTransferGenericFailure(to, amount);
        }
    }

In general, all transfer methods (transfer of ERC20, ERC721, ERC1155) will check to address should not be address(0): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/ERC20.sol#L232 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L336 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/ERC1155.sol#L167

But _transferEth doesn't check the to address. An offerer will lose ETH when the offerer try to transfer ETH to additional recipients but doesn't set the recipient address correctly: https://github.com/code-423n4/2022-05-opensea-seaport/blob/main/contracts/lib/BasicOrderFulfiller.sol#L962-L966

            // Transfer Ether to the additional recipient.
            _transferEth(
                additionalRecipient.recipient,
                additionalRecipientAmount
            );

Tools Used

None

Recommended Mitigation Steps

Add a null check for to address:

        require(to != address(0), "ETH: transfer to the zero address");

0age commented 2 years ago

This adds appreciable overhead in aggregate, prevents fulfillments that involve "burning" some portion of ETH, and would be highly unusual to occur in practice (i.e. including an amount but no recipient address).

HardlyDifficult commented 2 years ago

Merging with the warden's QA report #123

code-423n4 / 2022-05-opensea-seaport-findings