NFTBridge
60,000 USDC
View results
Submission Details
Severity: low
Valid

Tokens withdrawn from the bridge are not cleared from the escrow map in `Bridge::withdraw_auto_from_l1`, causing a state inconsistency and permanently removing `token_id` from circulation in `use_deposit_burn_auto` mode

Summary

Tokens withdrawn from the bridge are not cleared from the escrow map in Bridge::withdraw_auto_from_l1.

Vulnerability Details

An escrow map is defined as a map between collection's token_id and its depositor:

bridge.cairo#L71-L73

#[storage]
struct Storage {
...
// Registry of escrowed token for collections.
// <(collection_l2_address, token_id), original_depositor_l2_address>
escrow: LegacyMap::<(ContractAddress, u256), ContractAddress>,
}

Bridge::escrow_deposit_tokens(...) is called to deposit tokens into the escrow, which updates the current owner of the token_id in the escrow map:

bridge.cairo#L419

fn escrow_deposit_tokens(
ref self: ContractState,
contract_address: ContractAddress,
from: ContractAddress,
token_ids: Span<u256>,
) {
...
loop {
if i == token_ids.len() {
break ();
}
let token_id = *token_ids[i];
erc721.transfer_from(from, to, token_id);
@>> self.escrow.write((contract_address, token_id), from);
i += 1;
};
}

Bridge::withdraw_auto_from_l1(...), decorated with [L1_handler]which are special functions that can only be executed by a L1HandlerTransaction, processes message from L1 to withdraw tokens. It read the escrow map to check if a token_id is in escrow and if so, it transfers the token from the contract to its owner, otherwise it mints the token for that owner:

0bridge.cairo#L157

#[l1_handler]
fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
...
let mut i = 0;
loop {
if i == req.ids.len() {
break ();
}
...
@>> 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);
} 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);
}
}
i += 1;
};
...
}

The problem is that the function never clear the escrow map after withdrawal, which means that even after a token has been withdrawn, the escrow map will still show the token_id as being in escrow. Take the following scenario:

  • Alice initially bridges token_x from L1 to L2. Since is_escrowed is initially false, token_x is minted from the bridge to Alice.

  • Alice bridges token_x back to L1 by calling Bridge::deposit_tokens(...), which transfers token_x from Alice to escrow. is_escrowed is now true on L2. Alice doesn't own the token now on L2.

  • Alice bridges the token back to L2 and since is_escrowed is true on L2, this piece of code will be triggered:

let to = req.owner_l2;
let from = starknet::get_contract_address();
...
if is_escrowed {
IERC721Dispatcher { contract_address: collection_l2 }
.transfer_from(from, to, token_id);
...
}

As we see, token_id is transferred from the escrow to Alice but is_escrowed remains true. Aside from this state inconsistency, this problem is worse if token_id was burned from the escrow (in use_deposit_burn_auto mode) because the function will try to transfer the non-existing token from the escrow instead of minting it which will always revert because well, token_id does not exist, permanently removing that token_id from circulation.

Impact

  • State inconsistency: is_escrowed is true when the token is not in the escrow.

  • Permanent removing oftoken_id from circulation in use_deposit_burn_auto mode.

Tools Used

Manual review.

Recommendations

let to = req.owner_l2;
let from = starknet::get_contract_address();
...
if is_escrowed {
IERC721Dispatcher { contract_address: collection_l2 }
.transfer_from(from, to, token_id);
+ self.escrow.write((contract_address, token_id), 0);
...
}
Updates

Lead Judging Commences

n0kto Lead Judge 11 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

x18a6 Submitter
11 months ago
n0kto Lead Judge
11 months ago
n0kto Lead Judge 11 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.