Summary
When withdraw_auto_from_l1 is triggered and collection_l2 returns a non-zero value, it is possible that the collection_l2 has been removed from the whitelist. However, the whitelist status of collection_l2 is not checked, allowing the users to still interact with non-whitelisted collections.
Vulnerability Details
Bridge owner can add list of collections that is whitelisted, means only those collections that are allowed to bridged by users when white_list_enabled is set to true.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L317-L324
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L491-L542
fn white_list_collection(ref self: ContractState, collection: ContractAddress, enabled: bool) {
ensure_is_admin(@self);
_white_list_collection(ref self, collection, enabled);
self.emit(CollectionWhiteListUpdated {
collection,
enabled,
});
}
fn _white_list_collection(ref self: ContractState, collection: ContractAddress, enabled: bool) {
let no_value = starknet::contract_address_const::<0>();
let (current, _) = self.white_listed_list.read(collection);
if current != enabled {
let mut prev = self.white_listed_head.read();
if enabled {
self.white_listed_list.write(collection, (enabled, no_value));
if prev.is_zero() {
self.white_listed_head.write(collection);
return;
}
loop {
let (_, next) = self.white_listed_list.read(prev);
if next.is_zero() {
break;
}
let (active, _) = self.white_listed_list.read(next);
if !active {
break;
}
prev = next;
};
self.white_listed_list.write(prev, (true, collection));
} else {
if prev == collection {
let (_, next) = self.white_listed_list.read(prev);
self.white_listed_list.write(collection, (false, no_value));
self.white_listed_head.write(next);
return;
}
loop {
let (active, next) = self.white_listed_list.read(prev);
if next.is_zero() {
break;
}
if !active {
break;
}
if next == collection {
let (_, target) = self.white_listed_list.read(collection);
self.white_listed_list.write(prev, (active, target));
break;
}
};
self.white_listed_list.write(collection, (false, no_value));
}
}
}
However, when withdraw_auto_from_l1 is triggered by sequencer and collection_l2 returns a non-zero value from verify_collection_address when calling ensure_erc721_deployment, it is possible that the collection_l2 is already removed from whitelist due to various reasons. However, the whitelist status of collection_l2 is not checked, allowing the users to still interact with non-whitelisted collections.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L440-L442
fn ensure_erc721_deployment(ref self: ContractState, req: @Request) -> ContractAddress {
let l1_req: EthAddress = *req.collection_l1;
let l2_req: ContractAddress = *req.collection_l2;
let collection_l2 = verify_collection_address(
l1_req,
l2_req,
self.l2_to_l1_addresses.read(l2_req),
self.l1_to_l2_addresses.read(l1_req),
);
if !collection_l2.is_zero() {
>>> return collection_l2;
}
let hash = *req.hash;
let salt_data: Span<felt252> = array![hash.low.into(), hash.high.into()].span();
let salt = poseidon_hash_span(salt_data);
}
Impact
This means collection_l2 that is removed from whitelist, users can still interact with it even when white_list_enabled is set to true.
Tools Used
Manual review
Recommendations
Consider to check if collection_l2 is whitelisted when the value is non-zero.
fn ensure_erc721_deployment(ref self: ContractState, req: @Request) -> ContractAddress {
let l1_req: EthAddress = *req.collection_l1;
let l2_req: ContractAddress = *req.collection_l2;
let collection_l2 = verify_collection_address(
l1_req,
l2_req,
self.l2_to_l1_addresses.read(l2_req),
self.l1_to_l2_addresses.read(l1_req),
);
if !collection_l2.is_zero() {
+ assert(_is_white_listed(@self, collection_l2), 'Collection not whitelisted');
return collection_l2;
}
let hash = *req.hash;
let salt_data: Span<felt252> = array![hash.low.into(), hash.high.into()].span();
let salt = poseidon_hash_span(salt_data);
// ...
}
