search

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

7 stars 7 forks source link

Protocol is not ERC-1155 compliant #299

Open c4-bot-10 opened 8 months ago

c4-bot-10 commented 8 months ago

Lines of code

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

Vulnerability details

Impact

The Ocean contract is not fully compliant with the ERC-1155 standard. Specifically, the onERC1155Received and onERC1155BatchReceived functions do not revert when they reject a transfer. According to the ERC-1155 standard, these functions MUST revert if they reject a transfer. This may have been a deliberate choice by the authors given the comments in the functions' documentation, but ERC-1155 adherence is specifically listed as an invariant and this behaviour represents strict non-conformity.

Proof of Concept

This could lead to a potential loss of funds if an external IERC1155 contract is improperly implemented and doesn't check for the return value of onERC1155Received. In such a case, a user could unknowingly transfer tokens to the Ocean contract without a proper interaction. Since the Ocean contract doesn't revert the transaction, the tokens would be transferred successfully, but without the required interaction to credit them to the user in the ocean. This could lead to the tokens being stuck in the Ocean contract, resulting in a loss of funds for the user.

Tools Used

Manual inspection

Recommended Mitigation Steps

To ensure full compliance with the ERC-1155 standard, modify the onERC1155Received and onERC1155BatchReceived functions to revert when a transfer is rejected. This can be achieved by replacing the return 0; statement with a revert statement.

Assessed type

Other

c4-pre-sort commented 8 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 8 months ago

raymondfam marked the issue as duplicate of #218

c4-judge commented 8 months ago

0xA5DF marked the issue as not a duplicate

c4-judge commented 8 months ago

0xA5DF marked the issue as primary issue

c4-judge commented 8 months ago

0xA5DF changed the severity to QA (Quality Assurance)

0xA5DF commented 8 months ago

This could lead to a potential loss of funds if an external IERC1155 contract is improperly implemented and doesn't check for the return value of onERC1155Received. In such a case, a user could unknowingly transfer tokens to the Ocean contract without a proper interaction.

This is a loss of funds due to a mistake on the user's side. Marking as low

0xA5DF commented 8 months ago

Moved to #336

c4-judge commented 8 months ago

0xA5DF marked the issue as grade-c

c4-judge commented 8 months ago

0xA5DF marked the issue as grade-a