Summary
Remove from whitelist on L2 result in an infinite loop, because prev element is not updated.
Vulnerability Details
On L2(Starknet), collections can be removed from the whitelist by the admin via bridge.white_list_collection() with enable = false.
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 {
... Add case
} 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));
}
}
}
When a collection is about to be removed, it will enter the else statement, then check if the collection to be removed is the head of the linked list (this is how collections are stored), and if not, loop until found it. Now though, if the collection is not the head it will take the head.next element and check if next == collection and if not it should continue to the next in the list, but prev is never updated and this results in an infinite loop and always will only check for the first and second elements.
Coded Poc
Add the test to src/tests/bridge_t.cairo and run it with:
snforge test starklane::tests::bridge_t::tests::whitelist_collection_cannot_be_removed --exact
#[test]
fn whitelist_collection_cannot_be_removed() {
let erc721b_contract_class = declare("erc721_bridgeable");
let BRIDGE_ADMIN = starknet::contract_address_const::<'starklane'>();
let BRIDGE_L1 = EthAddress { address: 'starklane_l1' };
let bridge_address = deploy_starklane(BRIDGE_ADMIN, BRIDGE_L1, erc721b_contract_class.class_hash);
let bridge = IStarklaneDispatcher { contract_address: bridge_address };
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN);
bridge.enable_white_list(true);
stop_prank(CheatTarget::One(bridge_address));
let collection1 = starknet::contract_address_const::<'collection1'>();
let collection2 = starknet::contract_address_const::<'collection2'>();
let collection3 = starknet::contract_address_const::<'collection3'>();
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN);
bridge.white_list_collection(collection1, true);
bridge.white_list_collection(collection2, true);
bridge.white_list_collection(collection3, true);
stop_prank(CheatTarget::One(bridge_address));
let white_listed = bridge.get_white_listed_collections();
assert_eq!(white_listed.len(), 3, "White list shall contain 3 elements");
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN);
bridge.white_list_collection(collection3, false);
stop_prank(CheatTarget::One(bridge_address));
}
Impact
Only the head and second element of the list can be removed from the whitelist, everything else leads to an infinite loop.
Tools Used
Manual Review
Recommendations
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;
}
// find last element
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 {
// change head
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;
}
// removed element from linked list
loop {
let (active, next) = self.white_listed_list.read(prev);
if next.is_zero() {
// end of list
break;
}
if !active {
break;
}
if next == collection {
let (_, target) = self.white_listed_list.read(collection);
self.white_listed_list.write(prev, (active, target));
break;
}
+ prev = next
};
self.white_listed_list.write(collection, (false, no_value));
}
}
}
