Summary
The linked-list for the whitelist on L2 is incorrectly implemented, preventing admins from removing any collections after the second one from the linked list (whitelist).
Vulnerability Details
Currently the whitelist on L2 is implemented as a linked list. This implementation adds new elements at the end of it and allows for removing arbitrary elements and always being up to date with which collections are whitelisted.
To add/remove elements, the function _white_list_collection is used with the enabled flag set to either true for adding or false for removing.
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 {
} 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));
}
}
}
The logic for adding an element is fine but if we look at the part where we remove elements from the list, there is an issue.
Going through the logic, we want to do the following:
check if the element to remove is the head
if this is the case, we just update the head to point to the previous head's next element
if this is NOT the case, we loop over the list, looking for collection
once we find our collection in the list we unlink (remove) it from the list
if we do not find the collection in the list, we just return
Now the problem lies in the loop that traverses the linked list. More concretely, the prev element never gets updated to the current found element.
This means that we do the following:
check the head as above
if we enter the loop, we check the head's next element (as seen at [1])
we do NOT update the prev element so once we get to the next iteration, we basically go to 2) again, checking the head's next element again
This means we are stuck in an endless loop, meaning execution of the function will fail.
Impact
The impact of this is that admins cannot remove collections after the second one from the whitelist. This is quite severe as adding and removing collections from the whitelist is a critical part of the bridge.
Since the bridge is expected to be usable for more tokens other than Everai, this will be the case.
Proof of Concept
In order to show that elements after the second cannot be removed, please add the following test to bridge_t.cairo and execute it with snforge test third_whitelisted_cannot_be_removed from the apps/blockchain/starknet/ directory. This will fail showing that we cannot remove the wanted collection from the whitelist.
#[test]
fn third_whitelisted_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);
stop_prank(CheatTarget::One(bridge_address));
let white_listed = bridge.get_white_listed_collections();
assert_eq!(white_listed.len(), 1, "White list shall contain 1 element");
assert_eq!(*white_listed.at(0), collection1, "Wrong collection address in white list");
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN);
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");
assert_eq!(*white_listed.at(0), collection1, "Wrong collection address in white list");
assert_eq!(*white_listed.at(1), collection2, "Wrong collection address in white list");
assert_eq!(*white_listed.at(2), collection3, "Wrong collection address in white list");
assert!(bridge.is_white_listed(collection1), "Collection1 should be whitelisted");
assert!(bridge.is_white_listed(collection2), "Collection2 should be whitelisted");
assert!(bridge.is_white_listed(collection3), "Collection3 should be whitelisted");
println!("Before trying to remove collection3");
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN);
bridge.white_list_collection(collection3, false);
stop_prank(CheatTarget::One(bridge_address));
println!("After removing collection3");
let collection_none = starknet::contract_address_const::<0>();
assert_eq!(white_listed.len(), 2, "White list shall contain 2 elements");
assert_eq!(*white_listed.at(0), collection1, "Wrong collection address in white list");
assert_eq!(*white_listed.at(1), collection2, "Wrong collection address in white list");
assert_eq!(*white_listed.at(2), collection_none, "Wrong collection address in white list");
}
Tools Used
Manual review
Recommended Mitigation
This can be easily fixed by updating the prev element when traversing the list by adding a single line like so:
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;
}
@> prev = next;
};
self.white_listed_list.write(collection, (false, no_value));
}
}
}
