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

Some collections cannot be blacklisted/disabled on the L2 bridge

audinarey

Summary

The _white_list_collection(...)function called internally by white_list_collection(...) is used to whitelist/blacklist a collection. However the function is wrongly implemented and will be stuck in an infinite loop depending on the position of the collection to be disabled / blacklisted thus breaking core protocol functionality and leading to gas exhaustion (DOS).

Vulnerability Details

As shown below, the list of whitelisted collections always has a collection a the head stored in the white_listed_headcached on L496 as prevand the collection in this position is updated whenever the collection in that position is updated. Also, the white_listed_listcontains all the whitelisted tokens.

File: bridge.cairo
492: fn _white_list_collection(ref self: ContractState, collection: ContractAddress, enabled: bool) {
493: let no_value = starknet::contract_address_const::<0>(); // address zro
494: let (current, _) = self.white_listed_list.read(collection); // (curent status of the collection, next after the collection)
495: if current != enabled { // if tthe collection does not already have the status it is being set to
496: @> let mut prev = self.white_listed_head.read(); // the current address at the whitelist head
497: if enabled { // setting to enable
​
SNIP: ........
513: prev = next;
SNIP: ........
​
516: } else { // setting to false (blacklisting)
517: // change head
518: if prev == collection {
519: let (_, next) = self.white_listed_list.read(prev);
520: self.white_listed_list.write(collection, (false, no_value));
521: self.white_listed_head.write(next);
522: return;
523: }
524: // removed element from linked list
525: >>> loop {
526: let (active, next) = self.white_listed_list.read(prev);
527: if next.is_zero() {
528: // end of list
529: break;
530: }
531: if !active {
532: break;
533: }
534: if next == collection {
535: let (_, target) = self.white_listed_list.read(collection);
536: self.white_listed_list.write(prev, (active, target));
537: break;
538: }
539: };
540: self.white_listed_list.write(collection, (false, no_value));
541: }
542: }
543: }
​

Collection are added as shown in the sequence illustrated below in a linked list, (where nextis the end of the list and is initially set to zero until a new collection is added)

// The structure is as follows
|(col, next), (col, next), (col, next), (col, next)|
​
// for example
|c1, 0x0|
|(c1, c2), (c2, 0x0)|
|(c1, c2), (c2, c3), (c3,0x0)|
|(c1, c2), (c2, c3), (c3,c4), (c4,0x0), ....|
...

Each collection has its next pair (per se)
when blacklisting, L516 is executed and there is no problem with blaclisting c1 and c2.

However, when disabling c3and higher index collections, the loop on L525 will be executed and as follows

  • Admin wants to disable c3

  • on L496 prev == c1

  • L526 is executed and next == c2

  • previs not updated to the nextvalue of c2and as such when the loop iterates a second time, prev == c1 and next == c2

  • c3is never accessed and the loop keeps on iterating and is stuck in an infinite loop leading to a DOS in the contract.

Notice that this missing update was done during whitelisting on L513.

The reason why the test passed for 3 collections was because the 3rd collection was never removed first and as such this scenario was not caught in the tests.

Impact

  • Disabling/blacklisting some collections will not be successful and could cause the function to be stuck in an infinite loop and perhaps a DOS. This breaks core protocol functionality.

Tools Used

Manual review.

Recommendations

Modify the _white_list_collection(...)function as shown below

File: bridge.cairo
492: fn _white_list_collection(ref self: ContractState, collection: ContractAddress, enabled: bool) {
493: let no_value = starknet::contract_address_const::<0>(); // address zro
494: let (current, _) = self.white_listed_list.read(collection); // (curent status of the collection, next after the collection)
495: if current != enabled { // if tthe collection does not already have the status it is being set to
496: @> let mut prev = self.white_listed_head.read(); // the current address at the whitelist head
497: if enabled { // setting to enable
​
SNIP: ........
513: prev = next;
SNIP: ........
​
516: } else { // setting to false (blacklisting)
517: // change head
518: if prev == collection {
519: let (_, next) = self.white_listed_list.read(prev);
520: self.white_listed_list.write(collection, (false, no_value));
521: self.white_listed_head.write(next);
522: return;
523: }
524: // removed element from linked list
525: loop {
526: let (active, next) = self.white_listed_list.read(prev);
527: if next.is_zero() {
528: // end of list
529: break;
530: }
531: if !active {
532: break;
533: }
534: if next == collection {
535: let (_, target) = self.white_listed_list.read(collection);
536: self.white_listed_list.write(prev, (active, target));
537: break;
538: }
+539: prev = next;
540: };
541: self.white_listed_list.write(collection, (false, no_value));
542: }
543: }
544: }
​
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.