ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
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

x18a6

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 over 1 year 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
over 1 year ago
n0kto Lead Judge
over 1 year ago
n0kto Lead Judge over 1 year 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.

Give us feedback!