ArkProject: NFT Bridge

Summary

The _withdrawFromEscrow function in Escrow.sol does not follow the Checks-Effects-Interactions (CEI) pattern, which introduces a potential reentrancy risk, especially concerning ERC1155 tokens.

Vulnerability Details

In the current implementation, the state update _escrow[collection][id] = address(0x0); is performed after the transfer of tokens. This deviates from the CEI pattern, which recommends updating the state before performing external calls (like token transfers) to mitigate reentrancy risks. While this issue is not currently exploitable, adhering to the CEI pattern is a best practice that reduces the risk of reentrancy attacks in the future.

The code is not currently exploitable because the two flows that enter the _withdrawFromEscrow function are either through withdrawTokens or from cancelRequest - both calling consumeMessageFromL2 and cancelL1ToL2Message respectively which essentially create a reentrancy guard.

Impact

Although the current code does not allow for immediate exploitation, failure to follow the CEI pattern could make the contract more vulnerable to reentrancy attacks if the contract's behavior changes or if new features are added.

Recommendations

Refactor the _withdrawFromEscrow function to update the state before performing the token transfer. This change aligns with the CEI pattern and mitigates potential reentrancy risks:

This modification ensures that the contract state is consistent before any external interactions, reducing the likelihood of reentrancy issues.