NFTBridge
60,000 USDC
View results
Submission Details
Severity: high
Invalid

Potential Reentrancy Vulnerability in `withdraw_auto_from_l1` Function

Summary:

This function makes external calls to ERC721 contracts for transferring or minting tokens after modifying important state, violating the checks-effects-interactions pattern. This implementation creates a risk where malicious contracts could re-enter the function before its initial execution completes, potentially leading to double-spending of tokens or inconsistent state. Given the cross-chain nature of the bridge, this vulnerability could have significant impacts on both Ethereum and Starknet ecosystems, potentially resulting in financial losses and compromising the integrity of the bridging mechanism.

Vulnerability Details:
The withdraw_auto_from_l1 function in the bridge contract performs external calls to other contracts after modifying important state. Specifically, it calls transfer_from, mint_from_bridge_uri, or mint_from_bridge on external contracts without following the checks-effects-interactions pattern. This creates a potential reentrancy vulnerability.

The vulnerable code snippet:

if is_escrowed {
IERC721Dispatcher { contract_address: collection_l2 }
.transfer_from(from, to, token_id);
} else {
if (req.uris.len() != 0) {
let token_uri = req.uris[i];
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge_uri(to, token_id, token_uri.clone());
} else {
IERC721BridgeableDispatcher { contract_address: collection_l2 }
.mint_from_bridge(to, token_id);
}
}

These external calls are made after the function has processed part of the withdrawal request but before it has completed. If the called contracts are malicious or compromised, they could potentially call back into withdraw_auto_from_l1, leading to unexpected behavior.

Impact:

  1. Double-spending of tokens: An attacker could potentially withdraw the same tokens multiple times.

  2. Inconsistent state: The bridge's state could become inconsistent with the actual token ownership on L2.

  3. Drain of escrowed tokens: In the worst case, an attacker could drain all escrowed tokens from the bridge.

This vulnerability undermines the core functionality of the bridge and could lead to significant financial losses for users and the protocol.

Tools Used:

  • Manual code review

  • Static analysis of the Cairo smart contract

Recommendations:

  1. Implement the checks-effects-interactions pattern:

    • Perform all state changes before making external calls.

    • Consider using a two-step withdrawal process where the first step marks tokens as "withdrawn" and the second step performs the actual transfer.

  2. Use reentrancy guards:

    • Implement a reentrancy guard mechanism similar to OpenZeppelin's nonReentrant modifier in Solidity.

    • This could involve setting a state variable at the beginning of the function and resetting it at the end, with checks to prevent reentry.

  3. Consider using pull-over-push pattern:

    • Instead of directly transferring tokens, mark them as available for withdrawal.

    • Implement a separate function for users to claim their withdrawn tokens.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.