Some NFT could able to by-pass 'charged fee` while un-wrapping their NFT

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L903-L906 https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L920-L934

Vulnerability details

Impact

Un-wrapping fee could be bypassed with some special type of NFT token

Proof of Concept

In current code base UnwrapFee is applied when ERC20, ERC1155, ETh unwrapping occurs This fee is calculated via _calculateUnwrapFee()

Fee is not applicable in case of ERC721

Many real-world NFT token may support both ERC721 and ERC1155 standards, which may break this fee taking logic. For Ex-: The asset token of The Sandbox Game supports both ERC1155 and ERC721

So what happens here that (all below function call initiated from _doIntraction() or _doMultipleInteraction(). For simplicity i'm mentioning affected function here) -> A user wrap 5 token of sameId via _erc1155Wrap() -> To get rid from paying un-wrapping fee -> in suitable condition he unWrape his token one by one via calling _erc721Unwrap()

Tools Used

Manual Review

Recommended Mitigation Steps

Should re-consider this.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Fee is not applicable in case of ERC721 as you said.

[0xhacksmithh]

Fee is not applicable in case of ERC721 as you said.

Yaa i'm saying here that Caller wrap his tokens via wrap1155() and unwrap via unwrap721() one-by-one as for 721 fee is not applied. So he can save fee that will be charged if he unwrap his tokens via unWrap1155().

[c4-judge]

0xA5DF marked the issue as satisfactory

[0xA5DF]

I'd like to get @viraj124's input on this It seems like it'd work but I doubt if it'd be worth it for the attacker.

[viraj124]

this is expected protocol behaviour and unwrap fee anyways is very low

[c4-sponsor]

viraj124 (sponsor) disputed

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xA5DF]

Thanks! Marking as low due to not being very practical (not necessarily worth the gas) and also seems to be intended design

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Low quantity (combined with #288)