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

Disabling a Collection's Whitelist Aftter Previous Whitelisting Fails Everytime

z3r0

Summary

Attempting to disable a collection whitelist after a previous enabling/whitelisting fails as long as the latest 2 entries in the white_listed_list mapping are both set to true (i.e whitelisted) and are both not entries for the collection whitelist that is to be de-whitelisted.

Vulnerability Details

In bridge.cairo - the function _white_list_collection() which is responsible for whitelisting and de-whitelisting has a flaw when a previously whitelisted contract collection is to be de-whitelisted:

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;
} // @audit: previous is not incremented like it is done in the previous if block. // prev = next; should be used in the loop to continue
};
self.white_listed_list.write(collection, (false, no_value));
}
}

To understand the issue, lets look at the part of the code responsible for whitelisting a contract collection:

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));
}

To ensure progression within the loop, the prev variable is updated with next, this ensures progression within the loop if the first iteration does not meet the conditions to break out of the loop. This is highly necessary to avoid an infinite loop that repeats the current iteration when it doesn't break out. To simplify, look at the following code:

int i = 0;
while i != 5 {
// do something
i = i + 1; // without this, the loop repeats forever.
}

Now, take a look at the part of the code responsible for turning whitelist status from true to false:

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;
}
// @audit: previous is not incremented like it is done in the previous if block.
// @audit: `prev = next;` should be used in the loop to continue the loops main progression if this current iteration is not sufficient
};
self.white_listed_list.write(collection, (false, no_value));
}

As I highlighted with comments, the main loop increment does not happen, thus if this first iteration of the loop is not sufficient to break the loop - the loop gets stuck repeating the same conditions which just leads to an infinite loop evaluating with same parameters over and over.

Impact

Admin cannot de-whitelist a contract collection if the current list head is not the collection to de-whitelist.

Tools Used

Manual

Recommendations

Simply update the code with prev = next;:

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; // @audit-fix: previous is incremented and now the loop can continue beyond the first iteration that doesn't terminate it
};
self.white_listed_list.write(collection, (false, no_value));
}
Updates

Lead Judging Commences

n0kto Lead Judge 9 months 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.