Check to address should not equal to address(0)

[code423n4]

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]

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]

Merging with the warden's QA report #123