ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

Escrowed NFTs are not burned when withdrawing them from `L2Bridge`.

alqaqa

Vulnerability Details

In L1Bridge when we are withdrawing NFTs, and these NFTs are held by the Bridge, we transfer them from the Bridge to the receiver, then we burn that NFT by setting escrow address to zero.

Escrow.sol#L76-L86

function _withdrawFromEscrow( ... ) ... {
...
​
address from = address(this);
​
if (collectionType == CollectionType.ERC721) {
1: IERC721(collection).safeTransferFrom(from, to, id);
} else {
// TODO:
// Check here if the token supply is currently 0.
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
​
2: _escrow[collection][id] = address(0x0);
​
return true;
}

But in L2Bridge escrowed NFT token is not burned when transferring, we are just transferring the NFT without resetting the escrow mapping to address_zero.

bridge.cairo#L157-L162

let is_escrowed = !self.escrow.read((collection_l2, token_id)).is_zero();
​
if is_escrowed {
❌️ IERC721Dispatcher { contract_address: collection_l2 }
.transfer_from(from, to, token_id);
// @audit NFT is not reset to zero?!!
} else { ... }

So this will result in an NFT being escrowed by the L2Bridge (which is like being owned by him), But it is actually owned by another owner, which is the ownerL2 that received the tokens when Bridging tokens from L1 to L2.

Proof of Concept

  • UserA Bridged an NFT token from L2 to L1 via L2Bridge::deposit_tokens().

  • This NFT token is now escrowed by the L2Bridge and it holds it.

  • UserA received the NFT on L1 by his L1Address.

  • Time passed, and this NFT is traded between addresses.

  • This NFT is now getting Bridged from L1 to L2 via L1Bridge::depositTokens()

  • This NFT token is now escrowed by the L1Bridge and it holds it.

  • This token is getting withdrawn from L2Bridge.

  • Token was transferred to the ownerL2 but did not burn on L2Bridge (it is still escrowed).

  • NFT token is escrowed by L2Bridge, but it doesn't hold it.

  • NFT tokens is escrowed in both L1Bridge and L2Bridge.

Impacts

  • Incorrect State modification, where the NFT tokens will still be escrowed by L2Bridge but it doesn't hold them.

  • The NFTs will be escrowed in both L1Bridge and L2Bridge which breaks NFTs bridging invariant, where the NFT will exist in both Bridged in the time it should only be on only one Bridge (should be escrowed one chain).

Tools Used

Manual Review

Recommendations

Burn the NFT by setting its escrow to address_zero in L2Bridge.

diff --git a/apps/blockchain/starknet/src/bridge.cairo b/apps/blockchain/starknet/src/bridge.cairo
index 23cbf8a..85ec4d9 100644
--- a/apps/blockchain/starknet/src/bridge.cairo
+++ b/apps/blockchain/starknet/src/bridge.cairo
@@ -159,6 +159,7 @@ mod bridge {
if is_escrowed {
IERC721Dispatcher { contract_address: collection_l2 }
.transfer_from(from, to, token_id);
+ self.escrow.write((collection_l2, token_id), starknet::contract_address_const::<0>());
} else {
if (req.uris.len() != 0) {
let token_uri = req.uris[i];
Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

finding-L2-withdrawing-do-not-clean-escrow-mapping

Impact: Incorrect state without any other impact, which deserves a Low according to CodeHawks documentation.

Appeal created

alqaqa Submitter
9 months ago
n0kto Lead Judge
9 months ago
n0kto Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-L2-withdrawing-do-not-clean-escrow-mapping

Impact: Incorrect state without any other impact, which deserves a Low according to CodeHawks documentation.

Support

FAQs

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