Summary
Due to unbounded loop, admin can not change the whitelist status of a collection from true to false.
Vulnerability Details
The internal function _white_list_collection is invoked when a new L2 collection is deployed during withdrawal or the admin is changing the whitelisting status of a collection.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L473
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L319
In this function, if the whitelisting status of a collection is going to be changed from true to false, the else-clause will be executed.
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));
}
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L515
The issue is as follows:
Suppose there are three collections collection1, collection2, and collection3, and all of them are active (their whitelist status is true).
So, the head of the linked list is collection1.
Suppose admin is going to change the status of collection3 from true to false.
In the else-clause, the loop-body will be executed.
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;
}
};
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L524
In the first iteration, prev = collection1, next = collection2, and collection = collection3.
At the end of the first iteration, since prev is not updated, the loop repeats the steps again indefinitely. In other words, at the end of the loop the statement prev = next; is missing.
As a result, admin can not change the whitelist status of a collection from true to false if the collection is at least the third element in the linked list.
PoC
In the following test, three collection with status true are added. Then the status of the last collection is changed from true to false. Because of the infinite loop, the transaction reverts. To run the code use snforge test starklane::tests::bridge_t::tests::whitelist_collection_infinite_loop_when_collection_is_removed --exact.
#[test]
fn whitelist_collection_infinite_loop_when_collection_is_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");
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), "Collection1 should be whitelisted");
assert!(bridge.is_white_listed(collection3), "Collection1 should be whitelisted");
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN);
bridge.white_list_collection(collection3, false);
stop_prank(CheatTarget::One(bridge_address));
let white_listed = bridge.get_white_listed_collections();
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!(bridge.is_white_listed(collection1), "Collection1 should be whitelisted");
assert!(bridge.is_white_listed(collection2), "Collection1 should not be whitelisted");
assert!(!bridge.is_white_listed(collection3), "Collection1 should be whitelisted");
}
The output is:
[FAIL] starklane::tests::bridge_t::tests::whitelist_collection_infinite_loop_when_collection_is_removed
Failure data:
Got an exception while executing a hint: Hint Error: Error in the called contract (0x03b24bdfb3983f3361a7f81e871041cc45f3e1c21bfe3f1cbcaf7bec224627d5):
Error at pc=0:17590:
Could not reach the end of the program. RunResources has no remaining steps.
Cairo traceback (most recent call last):
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Unknown location (pc=0:17592)
Impact
Tools Used
Recommendations
Following is recommended:
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;
};
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L524
