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

Issue with removing an active element from the linked list

fyamf

Summary

Protocol uses the if-condition if !active {...} during removing an active element from the linked list to handle the situation properly if an inactive element by mistake (or any other bugs) is present in the linked list. But, it is not handling properly, and it leads to two situations:

  • The wrong inactive element would not be unlinked.

  • The active element after the to-be-removed element becomes unlinked.

Vulnerability Details

During whitelisting a collection from true to false, it iterates over all the elements to reach to the element which is going to be removed.

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

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L515

If between these iterations, it reaches to an element which is not active, it simply breaks the loop.

if !active {
break;
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L530

Then, the statement after the loop would be executed that simply sets the to-be-removed element to false.

self.white_listed_list.write(collection, (false, no_value));

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L539

The issue is that:
- the inactive element which was reached in between is not corrected.
- the element after the to-be-removed element would become un-linked.

Suppose the linked list is as follows:

  • collection1: true, next: collection2

  • collection2: false, next: collection3

  • collection3: true, next: collection4

  • collection4: true, next: 0

The goal is to set collection3 to false. During the iteration, when it reaches to collection2, due to its inactivity, it breaks the loop. Then, the state of the collection3 is changed to false, moreover collection4 becomes unlinked. The final status would be:

  • collection1: true, next: collection2

  • collection2: false, next: collection3

  • collection3: false, next: 0

  • collection4: true, next: 0

The correct status when the body of if !active {...} is executed should be:

  • collection1: true, next: collection4

  • collection2: false, next: 0

  • collection3: false, next: 0

  • collection4: true, next: 0

Please note that, it must never happen to have an inactive element in the linked list, in other words, the body of if !active {...} should never be executed. If it happened, it means another part of the code is broken. If for any reason (other part of the code is buggy, or an update misconfigures the list) the body of if !active {...} is executed to handle the buggy situation properly, but it leads to other bugs.

Impact

  • Breaking the linked list.

Tools Used

Recommendations

Following is recommended:

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 {
+ self.white_listed_list.write(prev, (false, no_value));
+ self.white_listed_list.write(oldPrev, (true, next));
break;
}
if next == collection {
let (_, target) = self.white_listed_list.read(collection);
self.white_listed_list.write(prev, (active, target));
break;
}
+ oldPrev = prev;
};
self.white_listed_list.write(collection, (false, no_value));
}

Or simply instead of breaking, assert should be used:

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;
- }
+ assert(active, "could not be inactive"); // here is added
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));
}
Updates

Lead Judging Commences

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

Support

FAQs

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