Summary
Because of the incorrect implementation of removing an element from a Linked List, removing whitelisted NFTs will not be possible.
Vulnerability Details
This issue is related to Linked List Data Structures.
The current Data Structure used for Storing WhiteListed NFTs on Starknet is Linked List. In Ethereum, we are using arrays, but the Protocol preferred to use LinkedList in Starknet.
When removing an element from a linked list, we need to start from the Head then go to the Next, then Next ... till we reach the element. But in the current removing logic, the protocol forgot to Move to the Next element when removing.
bridge.cairo#L523-L537
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;
}
// @audit where is `prev = next` step
}
As we can see from the loop we are just reading the prev element, which is the head when beginning iteration, check for the next element if it is zero, or not activated. and check for the next element to be the collection we need to remove, But when we finish the iteration, we are not moving a step forward i.e prev = next, so this will make us loop in an infinite loop, leading to the tx reverted in the last because of the consumption of all gas.
This is not the case when adding new element, where the team handled it correctly.
bridge.cairo#L512
// find last element
loop {
...
@> prev = next;
}
This will make removing any element in the list except first one in most cases unable to get removed.
Proof of Concept
We made a simple Script to demonstrate how removing an element will not succeed, and the tx will revert when doing this.
Add this test in apps/blockchain/starknet/src/tests/bridge_t.cairo.
#[test]
fn auditor_whitelist_collection_remove_collection_dos_issue() {
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'>();
let collection4 = starknet::contract_address_const::<'collection4'>();
let collection5 = starknet::contract_address_const::<'collection5'>();
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);
bridge.white_list_collection(collection4, true);
bridge.white_list_collection(collection5, true);
stop_prank(CheatTarget::One(bridge_address))
let white_listed = bridge.get_white_listed_collections();
assert_eq!(white_listed.len(), 5, "White list shall contain 5 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_eq!(*white_listed.at(3), collection4, "Wrong collection address in white list");
assert_eq!(*white_listed.at(4), collection5, "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");
assert!(bridge.is_white_listed(collection4), "Collection4 should be whitelisted");
assert!(bridge.is_white_listed(collection5), "Collection5 should be whitelisted");
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN)
bridge.white_list_collection(collection3, false);
stop_prank(CheatTarget::One(bridge_address))
}
To run it you can write this command on apps/blockchain/starknet path
snforge test auditor_whitelist_collection_remove_collection_dos_issue
Output:
[FAIL] starklane::tests::bridge_t::tests::auditor_whitelist_collection_remove_collection_dos_issue
Failure data:
Got an exception while executing a hint: Hint Error: Error in the called contract (0x03b24bdfb3983f3361a7f81e871041cc45f3e1c21bfe3f1cbcaf7bec224627d5):
Error at pc=0:7454:
Could not reach the end of the program. RunResources has no remaining steps.
Cairo traceback (most recent call last):
...
Impact
Inability to remove whitelisted collections.
If a specific whitelisted NFT collection is malicious, we cannot unwhitelist it.
The only way to remove a specific NFT will be to start removing from the head till we reach that NFT collection to be unwhitelisted which will affect other NFT collections and is not an applicable solution in production.
Tools Used
Manual Review + Starknet Foundry
Recommendations
Move to the next element in the linked list once completing the iteration.
@@ -535,6 +535,7 @@ mod bridge {
self.white_listed_list.write(prev, (active, target));
break;
}
+ prev = next;
};
self.white_listed_list.write(collection, (false, no_value));
}
