ArkProject: NFT Bridge
ArkProject
NFTBridge
60,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Admin won't be able to remove certain collections from the whitelist on L2

0xasen

Summary

Admin won't be able to remove collections from the whitelist on L2 in certain cases. The issue stems from the fact that there's a missing line in the _white_list_collection function in the bridge.cairo contract that will make the function loop indefinitely if the collection to be removed is after 2nd place in the linked list.

Removing a collection by an admin should always be possible. This will have an even bigger impact if a malicious collection finds its way into the whitelist either by accident or at a time that the whitelist has been off.

Vulnerability Details

The white_list_collection() function is used by admins to add or remove collections from the whitelist. It utilizes the internal _white_list_collection(). Let's take a look at it:

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 the current status is not the same as the desired status
if current != enabled {
let mut prev = self.white_listed_head.read();
//if adding to whitelist
if enabled {
//functionality for enabling a collection on the whitelist
}
​
//if disabling the collection
} else {
​
//if the collection is the head of the list
if prev == collection {
//read the next element in the list from the head
let (_, next) = self.white_listed_list.read(prev);
//write the current collection as disabled
self.white_listed_list.write(collection, (false, no_value));
//update the head of the list to the next element
self.white_listed_head.write(next);
//exit the function since the collection was at the head and is now removed
return;
}
​
//if the collection is not the head, remove it from the linked list
loop {
//read the current active status and the next element
let (active, next) = self.white_listed_list.read(prev);
//if the next element is zero, end of list is reached, break the loop
if next.is_zero() {
// end of list
break;
}
//if the current element is not active, break the loop
if !active {
break;
}
//if the next element is the collection to be removed
if next == collection {
//read the next element after the collection to be removed
let (_, target) = self.white_listed_list.read(collection);
//update the previous element to point to the element after the collection
self.white_listed_list.write(prev, (active, target));
break;
}
​
//here we should have prev = next; <---
};
​
self.white_listed_list.write(collection, (false, no_value));
}
}
}

Let's consider the following scenario:
We have a linked list structure of the following collections: A -> B -> C -> D and we want to remove C

  • let mut prev = self.white_listed_head.read(); sets prev to A.
    Entering the else branch since enabled is false and we're removing a collection from the whitelist.

  • if prev == collection is false since A != C so we go to the loop

First Loop Iteration:

  • let (active, next) = self.white_listed_list.read(prev); reads B (next of A).

  • Since B is not zero and active, these ifs are skipped if next.is_zero(), !active, and the function proceeds.

  • Since next (B) is not the target collection (C), this returns false if next == collection and the if is skipped.

At this point, the loop should move to the next element, but the missing line prev = next; is not there.
The loop incorrectly processes B again in the next iteration.

This will continue indefinitely since no break condition will be met, causing the function to revert because of out-of-gas at some point and making the target collection impossible to remove.

Impact

Admins can't remove certain collections from the whitelist breaking the core functionality of the _white_list_collection function.

Tools Used

Manual review

Recommendations

Add the prev = next line at the spot I've pointed out in the code above.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-L2-unwhitelist-from-third collection-impossible

Likelyhood: High, owner can only unwhitelist the 2 first collections. Impact: Medium/High, owner has to empty the list to remove any collection, and replace all the new ones.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.