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

Edge cases are not handled correctly in the `bridge.cairo::_white_list_collection` function

bube

Link

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L491-L542

Summary

The bridge.cairo::_white_list_collection function doesn't handle the edge cases when we want to remove the last element from the list and when the list contains only one element.

Vulnerability Details

The bridge.cairo::_white_list_collection function loops through the list of collections and removes element from the list:

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

In a linked list, the tail is the last element in the list, and it's typically important to track this last element to efficiently perform operations like appending new elements to the end of the list. If you only track the head (the first element) and need to find the tail (the last element) during operations like adding or removing elements, you may end up traversing the entire list, which can be inefficient.

The problem in that function is that the function doesn't track the tail of the list and doesn;t update the tail pointer if the removed element is the last collection from the list. There is also one more edge case when the list contains only one element and we want to remove it. In that case the function should ensure that the next element pointer is set to zero, but this is missing in the implementation of the function.

Impact

If the element to be removed is the last one in the list, the next pointer of the previous element should be set to zero to indicate the end of the list and the tail pointer should be updated to the previous element in the list. The current logic does not explicitly handle this scenario. If you don’t update it, the tail pointer would still reference the removed element, which no longer exists in the list.

If the list is empty (prev.is_zero()), the new collection becomes the head. If the collection to be removed is the head and there are no other elements, the head should be set to zero. When disabling the only element, the function should ensure that both the head and the element's next pointer are set to zero. But the current implementation doesn't ensure that.

Recommendations

After finding the target element and before breaking the loop, check if the next pointer of the target element is zero. If so, set the next pointer of the previous element to zero.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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