60,000 USDC
Submission Details
Severity:
withdraw_auto_from_l1 escrow is not zeroed
Summary
L2 escrow is not reset when withdrawn.
Vulnerability Details
When tokens are deposited on L2 via deposit_tokens(), they will be deposited in the escrow mapping, so when someone transfers back the same NFTs from L1 → L2 to take it from the contract balance, because they already exist.
fn deposit_tokens(
ref self: ContractState,
salt: felt252,
collection_l2: ContractAddress,
owner_l1: EthAddress,
token_ids: Span<u256>,
use_withdraw_auto: bool,
use_deposit_burn_auto: bool,
) {
ensure_is_enabled(@self);
assert(!self.bridge_l1_address.read().is_zero(), 'Bridge is not open');
// TODO: we may have the "from" into the params, to allow an operator
// to deposit token for a user.
// This can open more possibilities.
// Because anyway, a token can't be moved if the owner of the token
// is not giving approval to the bridge.
let from = starknet::get_caller_address();
assert(_is_white_listed(@self, collection_l2), 'Collection not whitelisted');
// TODO: add support for 1155 and check the detected interface on the collection.
let ctype = CollectionType::ERC721;
let erc721_metadata = erc721_metadata(collection_l2, Option::Some(token_ids));
let (name, symbol, base_uri, uris) = match erc721_metadata {
Option::Some(data) => (data.name, data.symbol, data.base_uri, data.uris),
Option::None => ("", "", "", array![].span())
};
escrow_deposit_tokens(ref self, collection_l2, from, token_ids); // AUDIT - Saved here
... More code
}
But when they are transferred back to L2, withdraw_auto_from_l1() transfers them if they exist in the escrow mapping, but never resets the mapping when transferring it, this creates confusion that the tokens are still in the bridge.cairo contract.
#[l1_handler]
fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
ensure_is_enabled(@self);
assert(self.bridge_l1_address.read().into() == from_address,
'Invalid L1 msg sender');
// TODO: recompute HASH to ensure data are not altered.
// TODO: Validate all fields the request (cf. FSM).
let collection_l2 = ensure_erc721_deployment(ref self, @req);
let _ctype = collection_type_from_header(req.header);
// TODO: check CollectionType to support ERC1155 + metadata.
let mut i = 0;
loop {
if i == req.ids.len() {
break ();
}
let token_id = *req.ids[i];
let to = req.owner_l2;
let from = starknet::get_contract_address();
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;
};
self.emit(WithdrawRequestCompleted {
hash: req.hash,
block_timestamp: starknet::info::get_block_timestamp(),
req_content: req
});
}
Impact
bridge.cairo::escrow mapping is never reset when tokens are transferred from it.
Tools Used
Manual Review
Recommendations
Reset the mapping when transferring from it.
#[l1_handler]
fn withdraw_auto_from_l1(
ref self: ContractState,
from_address: felt252,
req: Request
) {
ensure_is_enabled(@self);
assert(self.bridge_l1_address.read().into() == from_address,
'Invalid L1 msg sender');
// TODO: recompute HASH to ensure data are not altered.
// TODO: Validate all fields the request (cf. FSM).
let collection_l2 = ensure_erc721_deployment(ref self, @req);
let _ctype = collection_type_from_header(req.header);
// TODO: check CollectionType to support ERC1155 + metadata.
let mut i = 0;
loop {
if i == req.ids.len() {
break ();
}
let token_id = *req.ids[i];
let to = req.owner_l2;
let from = starknet::get_contract_address();
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);
+ let no_value = starknet::contract_address_const::<0>();
+ self.escrow.write((collection_l2, token_id), no_value);
} 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;
};
self.emit(WithdrawRequestCompleted {
hash: req.hash,
block_timestamp: starknet::info::get_block_timestamp(),
req_content: req
});
}
Updates
Lead Judging Commences
n0kto about 1 year ago
Submission Judgement Published 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.
Prize pool breakdown
Total prize
60,000 USDC
nSLOC
2,30126 USDC / LOC
50,000 USDC
5,500 USDC
4,500 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.