code-423n4 / 2023-11-shellprotocol-findings

7 stars 7 forks source link

`Ocean` cannot `_mintBatch()` as `onERC1155BatchRecieved()` not implemeneted on the `Ocean` contract when batch transferring to itself #261

Closed c4-bot-9 closed 9 months ago

c4-bot-9 commented 9 months ago

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L353-L366

Vulnerability details

The comment @ Ocean L348 states:

The Ocean never initiates ERC1155 Batch Transfers.

This is untrue, note the following callstack:

Ocean.doMultipleInteractions | Ocean.forwardedDoMultipleInteractions > Ocean._doMultipleInteractions (>> calls _mintBatch @ L560 <<) > OceanERC1155._mintBatch > OceanERC1155._doSafeBatchTransferAcceptanceCheck (will revert) > Ocean.onERC1155BatchReceived() (returns 0 if target is Ocean contract)

Additionally, the comments of the spec for onERC1155BatchReceived() prescribe: (docs)

Return of any other value than the prescribed keccak256 generated value MUST result in the transaction being reverted by the caller.

Since a revert is inevitable, it makes sense to decide weather or not the Ocean contract should accept these transfers or not, and if not just revert() instead of returning 0.

Recommended Mitigation Steps

The protocol team should determine if they want batch transfers to be accepted or rejected.

If accepted: Ideally this should return the proper selector and allow the transfer of multiple tokens.

If rejected: It would make sense for this function to revert() instead of returning 0 as any inproper selector MUST be reverted by the caller anyway. It makes sense to just revert in onERC1155Received() and onERC1155BatchReceived() here to save the end user gas and ensure compliance with the standard as any rejected transfer MUST revert.

Assessed type

Token-Transfer

c4-pre-sort commented 9 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 9 months ago

raymondfam marked the issue as duplicate of #21

c4-judge commented 9 months ago

0xA5DF marked the issue as unsatisfactory: Invalid