ArkProject: NFT Bridge

Relevant GitHub Links

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/token/TokenUtil.sol#L35

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/ethereum/src/Bridge.sol#L95

Summary

In this submission, I am going to addresses an issue where certain NFT tokens, which support both ERC721 and ERC1155 standards, incorrectly trigger TokenUtil::detectInterface to return CollectionType.ERC721. This behavior disrupts the functionality of Bridge::depositTokens.

Vulnerability Details

The current system does not account for ERC1155 tokens, leading to incorrect identification when tokens support both ERC721 and ERC1155 standards. Specifically, TokenUtil::detectInterface should identify such tokens as CollectionType.ERC1155 instead of CollectionType.ERC721. An example of this issue involves The Sandbox Game's(token address and implementation) asset token, a top 20 ERC1155 token in 2024, which supports both standards. When attempting to deposit these assets through Bridge::depositTokens, the system misidentifies them as ERC721 tokens due to the current logic in TokenUtil::detectInterface.

Assuming there is there is a user who tries to deposit of Sandbox's ASSETs, the actual deposit is carried by Bridge::depositTokens.

Bridge::depositTokens will call TokenUtil::detectInterface whcih will return CollectionType.ERC721

After that request is built and StarklaneEscrow::_depositIntoEscrow is called to save data in to escrow.

Inside StarklaneEscrow::_depositIntoEscrow, it will update the map of escrowed tokens.

If two users deposited the token asset with same id, i.e, alice deposited first and bob deposited later, bob will be the owner of the tokens and alice will be unable to withdraw token as she is not the owner anymore.

Impact

Users unfamiliar with the technical details of their NFTs may lose access to their tokens if they are mistakenly identified as ERC721 tokens. This could result in tokens being permanently locked within the bridge without recovery options, especially if the project does not utilize whitelists.

Tools Used

Mannual

Proof of Concept

Check the return values of Sandbox's ASSET's supportInterface, both supportInterface(0x80ac58cd) (type(IERC721).interfaceId is 0x80ac58cd) and supportInterface(0xd9b67a26)(type(IERC1155).interfaceId is 0xd9b67a26) return true.

Recommendations

Reorder the interface checks in TokenUtil::detectInterface