ArkProject: NFT Bridge

Summary

Vulnerability Detail

The CollectionManager contract is responsible for managing the deployment and verification of bridgeable ERC721 and ERC1155 contracts between Layer 1 (L1) and Layer 2 (L2). A critical function in this contract is _verifyRequestAddresses(), which verifies the mapping between L1 and L2 addresses. This function is designed to check if the provided L1 and L2 addresses match the stored mappings and revert if there's a mismatch.

However, there's a logical flaw in the address comparison logic within this function. Specifically, the condition if (l2Req > 0 && l1Req > address(0)) is problematic because address(0) is a valid address in Solidity, and comparing it with > leads to unexpected behavior. This flawed logic can cause the function to revert incorrectly, potentially blocking legitimate operations and preventing the deployment of new collections.

The root cause of this issue lies in the misunderstanding of how Solidity handles address comparisons. In Solidity, addresses are treated as uint160 when compared, so address(0) is effectively 0 when used in a comparison. The intention of the code was likely to check if the L1 address is non-zero, but the comparison l1Req > address(0) doesn't achieve this correctly.

Relevant code snippet:

Impact

The incorrect address verification logic can lead to unintended reverts, blocking legitimate operations and preventing the deployment of new collections. This can disrupt the normal functioning of the contract, especially in scenarios where address verification is critical. The issue can cause denial of service by preventing legitimate operations, eroding user trust in the contract.

Proof of Concept

1. A user initiates a bridging operation that calls _verifyRequestAddresses() with a valid L1 address (non-zero) and a valid L2 address.

2. The function reaches the condition if (l2Req > 0 && l1Req > address(0)).

3. Even though l1Req is a valid, non-zero address, the comparison l1Req > address(0) may not evaluate as expected due to the incorrect use of the > operator with addresses.

4. The function may incorrectly skip this block of code, leading to an unexpected ErrorVerifyingAddressMapping revert, or enter the block with unexpected results.

5. The bridging operation fails, preventing the user from completing their intended action.

Tools Used

Manual review

Recommendation

To fix this issue, the address comparison should be changed to use != instead of > when checking for non-zero addresses. This will correctly identify valid, non-zero addresses without the unexpected behavior of the current implementation.

Here's the recommended fix:

This change ensures that the function correctly identifies and handles non-zero addresses, preventing unintended reverts and allowing legitimate operations to proceed as expected.