search

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

1 stars 0 forks source link

QA Report #91

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure. Failure to do so will cause silent failures of transfers and affect token accounting in contract.

Reference: This similar medium-severity finding from Consensys Diligence Audit of Fei Protocol.

Instances:

reference/lib/ReferenceExecutor.sol:60: function _transfer( reference/lib/ReferenceOrderFulfiller.sol:228: _transfer( reference/lib/ReferenceOrderFulfiller.sol:281: _transfer( reference/lib/ReferenceOrderCombiner.sol:638: _transfer( reference/conduit/ReferenceConduit.sol:53: _transfer(standardTransfer); reference/conduit/ReferenceConduit.sol:96: _transfer(standardTransfer); reference/conduit/ReferenceConduit.sol:128: function _transfer(ConduitTransfer calldata item) internal { contracts/lib/Executor.sol:54: function _transfer( contracts/lib/OrderCombiner.sol:640: _transfer( contracts/conduit/Conduit.sol:71: _transfer(standardTransfer); contracts/conduit/Conduit.sol:135: _transfer(standardTransfer); contracts/conduit/Conduit.sol:174: function _transfer(ConduitTransfer calldata item) internal {

reference/lib/ReferenceTokenTransferrer.sol:82: ERC721Interface(token).transferFrom(from, to, identifier); contracts/interfaces/AbridgedTokenInterfaces.sol:5: function transferFrom( contracts/interfaces/AbridgedTokenInterfaces.sol:13: function transferFrom( contracts/lib/TokenTransferrerConstants.sol:48:// abi.encodeWithSignature("transferFrom(address,address,uint256)") contracts/test/TestERC20.sol:30: function transferFrom( contracts/test/TestERC20.sol:39: super.transferFrom(from, to, amount);

Recommended Mitigation Steps:

Consider using safeTransfer/safeTransferFrom or require() consistently.

HardlyDifficult commented 2 years ago

Merging with https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/92 and https://github.com/code-423n4/2022-05-opensea-seaport-findings/issues/93

HardlyDifficult commented 2 years ago

92 is referring to test files - so irrelevant.

93 is suggesting they are using timestamp for entropy, but that's not the case. It's use for time sensitive logic which is noted in the report as well, but there is not a viable alternative suggested.

The report above seems to be referencing the OZ implementation for ERC20 safeTransfer (although it does include lines pointing to 721 transfers as well). The ERC20 transfer logic in Seaport does already implement the same safe guards as the OZ implementation, so this finding seems to be n/a.

I'm sure the sponsor appreciates the findings, however, I think it is not fair to other wardens who submitted QA reports to have this included for rewards. I will be marking all low-quality QA reports as invalid.

0xleastwood commented 2 years ago

Warden submitted multiple QA reports. Will be excluded from judging.