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

Starkent bridge owner can NOT dewhitelist a collection that is in a greater position than the second in the linked list

draiakoo

Summary

Starkent bridge owner can NOT dewhitelist a collection that is in a greater position than the second in the linked list

Vulnerability Details

The way that a new collection either gets whitelisted or dewhitelisted is by first going through all the linked list. In the case of whitelist, it is going to loop until reaching the tail and then adding the new whitelisted collection there. In the case of dewhitelisting, it loops until it finds the collection, once there it changes the pointer of the previous collection to the next one and sets the collection to false (representing that is not whitelisted). The coded implementation looks like this:

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;
}
};
self.white_listed_list.write(collection, (false, no_value));
}
}
}

The issue here is that when it dewhitelists a collection (else branch) it can only handle the first 2 positions of the linked list, the head and the pointed position by the head. That is because it enters a loop that it does not get updated to following iterations and get stuck in an infinite loop. Let's visualize the error with a simple example:

current linked list
collection1 (head) -> collection2 -> collection3

Here the owner wants to dewhitelist the collection3. Hence, this part of the code will be executed:

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 {
// 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;
}
};
self.white_listed_list.write(collection, (false, no_value));
}
}
}

The prev variable will be collection1 because it is the head, the first if structure inside the else branch will be ignored because prev is not the collection3 and it will enter the loop. The first iteration will set active = true and next = collection2, since next is not zero and active is true the first 2 ifs will be ignored. The third if will also be ignored because next = collection2 and collection = collection3, so the loop will iterate again, but since the prev variables has not been changed/updated the loop will compute the same result and it will be stuck in an infinite loop.

Notice that even though it leads to an infinite loop in some situations, there are others where the transaction succeeds. These cases are when the collection that needs to be dewhitelisted are either the head or the pointed collection by the head. You can check that yourself.

A good point to mention is that in the test suite, there is a test where it dewhitelists 2 collections successfully.

fn whitelist_collection_is_updated_when_collection_is_removed() {
...
​
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));
​
start_prank(CheatTarget::One(bridge_address), BRIDGE_ADMIN);
bridge.white_list_collection(collection2, false);
bridge.white_list_collection(collection1, false);
bridge.white_list_collection(collection3, false);
stop_prank(CheatTarget::One(bridge_address));
​
...
}

In this test we can see that it dewhitelists all collections from the linked list, however in this test only dewhitelists either the head or the pointed position by the head. Hence, as explained, the function will work but not in the case I explained that the bug arises. That's the main reason why the tests did not catch this vulnerability.

You can check that by changing the test to dewhitelist the third collection instead in first place. You will get the error of the test failing.

Impact

The impact of this issue is high, because the owner can not dewhitelist a collection that is far from the head without having to do a lot of whitelisting and dewhitelisting before. Take the following example:
current linked list:

collection1 (head) -> collection2 -> collection3 -> collection4 -> collection5 -> collection6 -> collection7

If the owner wants to dewhitelist for example the collection6, he has to dewhitelist first the collection1, collection2, collection3 and collection4 (in this order) and then he will be able to dewhitelist the collection6. After that he will need to whitelist again all the collections 1, 2, 3 and 4. Also notice that this will mess the linked list order so if it is relevant for some reason it will also be an issue.

Tools Used

Manual review

Recommendations

The is just a single instruction that was forgotten:

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;
}
+ prev = next;
};
self.white_listed_list.write(collection, (false, no_value));
}
}
}
Updates

Lead Judging Commences

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

Give us feedback!