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

1 stars 0 forks source link

QA Report #185

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Low

Revert if additionalRecipientAmount is 0 ERC20

If additionalRecipient.amount == 0, the _transferERC20 call will revert. Not sure if it is intended, seemed unnecessary.

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/BasicOrderFulfiller.sol#L1054-L1061

            _transferERC20(
                erc20Token,
                from,
                additionalRecipient.recipient,
                additionalRecipientAmount,
                conduitKey,
                accumulator
            );

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/Executor.sol#L260-L269

    function _transferERC20(
        address token,
        address from,
        address to,
        uint256 amount,
        bytes32 conduitKey,
        bytes memory accumulator
    ) internal {
        // Ensure that the supplied amount is non-zero.
        _assertNonZeroAmount(amount);

Revert if additionalRecipientAmount is 0 ETH

If additionalRecipient.amount == 0, the _transferEth call will revert. Not sure if it is intended, seemed unnecessary.

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/BasicOrderFulfiller.sol#L963-L966

            _transferEth(
                additionalRecipient.recipient,
                additionalRecipientAmount
            );

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/Executor.sol#L222-L224

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

Non-Critical

Upgrade to latest Solidity

Consider upgrade to 0.8.14 released on 2022-05-17, but can be risky since a lot of low level assembly and compiler dependent behavior is used.

HardlyDifficult commented 2 years ago

Those reverts do appear to be intentionally included.

The new solc version was published when this contest was being posted so I think that was just a timing thing -- they later did perform the upgrade.

Since this feedback does not appear helpful, closing as invalid.

GalloDaSballo commented 1 year ago

Agree with judge