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

Users funds can be stuck in the L1 escrow without a way to withdraw/retrieve them

audinarey

Summary

Users who deposit into the L1 ocntract will not be able to withdraw their tokens on L2 and also will not be able to retrive it on L1 causing the tokens to be stuck in the L1 escrow

Vulnerability Details

Because the L1 Bridge::depositTokens(...)function allows any random collection to be deposited when _whiteListEnabled= false(is whitelist is disabled). A malicious user can DOS L2 withdrawal causing NFTs deposited on L1 to be stuck

File: Bridge.sol
334: function _isWhiteListed(
335: address collection
336: ) internal view returns (bool) {
337: @> return !_whiteListEnabled || _whiteList[collection];
338: }
​

A malicious user can deploy many random NFT collections on L1 when white list is disabled and deposit the them individually by calling Bridge::depositTokens(...)on L1 and then call bridge::withdraw_auto_from_l1(...) to withdraw them on the L2 with the deposited L1 collections.

Now, because these tokens have not been deployed on the L2, when bridge::withdraw_auto_from_l1(...)is called on the L2, the collections are deployed on the L2 and added to the collections list causing the loop on L504 below to be extremely large. The problem is that as shown in the link https://docs.starknet.io/tools/limits-and-triggers/, there is a block gas limit of 5,000,000 on starknet hence the loop will cause calls to withdraw_auto_from_l1(...)to exceed the L2 block gas limit and revert.


SUMMARY:

  • whitelist is disabled

  • Dan a malicious user deploys many NFT collections that were not otherwise whitelisted on L1

  • Dan deposits all of the collections considering payload size limit making the

  • Dan calls withdraw_auto_from_l1(...)from L1 and extends the white_listed_listmaking the loop on L504 longer until a single call to withdraw_auto_from_l1(...)can exceed 5,000,000 block gas limit (source: https://docs.starknet.io/tools/limits-and-triggers/) leading to gas exhaustion

  • Alice deposits Evarai in L1 (the transaction is successfully processed by the sequencer

  • Alice calls withdraw_auto_from_l1(...)from L1 but the transaction reverts because block gas limit has been exceeded

  • Now Alice has no way to retrive her NFT form L1 where it was initially deposited


Worse yet is that the deposit cannot be retrieved by even the Admin because it has suceessfully been added by the sequencer and cannot be cancelled form L1.

File: bridge.cairo
​
130: fn withdraw_auto_from_l1(
131: ref self: ContractState,
132: from_address: felt252,
133: req: Request
134: ) {
135: ...
141: //@audit the collection is added if not deployed on L2 already
142: @> let collection_l2 = ensure_erc721_deployment(ref self, @req);
​
​
​
429: fn ensure_erc721_deployment(ref self: ContractState, req: @Request) -> ContractAddress {
430:
431: let l1_req: EthAddress = *req.collection_l1;
432: let l2_req: ContractAddress = *req.collection_l2;
433:
...
470:
471: // update whitelist if needed
472: let (already_white_listed, _) = self.white_listed_list.read(l2_addr_from_deploy);
473: if already_white_listed != true {
474: @> _white_list_collection(ref self, l2_addr_from_deploy, true);
475: ....;
479: }
480: l2_addr_from_deploy
481: }
​
​
492: fn _white_list_collection(ref self: ContractState, collection: ContractAddress, enabled: bool) {
493: let no_value = starknet::contract_address_const::<0>(); // address zro
....
503: // find last element
504: @> loop {
505: let (_, next) = self.white_listed_list.read(prev); // the last collection that was whitelisted before this current one and its next
506: if next.is_zero() {
507: break;
508: }
509: let (active, _) = self.white_listed_list.read(next);
510: if !active {
511: break;
512: }
513: prev = next;
514: };
515: self.white_listed_list.write(prev, (true, collection));
​

This same attack can be performed by attacker to ensure funds are locked in the L2. So it goes both ways

Impact

NFT deposited in L1 will be stuck in the escrow leading to loss of funds for users.
Likelyhood: high
Impact: High (due to loss of NFT asset)

Tools Used

Manual review

Recommendations

Consider modifying

  • the Bridge::_isWhiteListed(...)function on L1 as shown below

File: Bridge.sol
334: function _isWhiteListed(
335: address collection
336: ) internal view returns (bool) {
-337: @> return !_whiteListEnabled || _whiteList[collection];
+337: @> return _whiteList[collection];
338: }

  • the bridge::_is_white_listed(...)function on L2 as shown below

File: bridge.cairo
483: fn _is_white_listed(self: @ContractState, collection: ContractAddress) -> bool {
-484: let enabled = self.white_list_enabled.read();
-485: if (enabled) {
-486: let (ret, _) = self.white_listed_list.read(collection);
-487: return ret;
-488: }
-489: true
+486: let (ret, _) = self.white_listed_list.read(collection);
+487: return ret;
490: }
​
Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-collections-always-withelisted-on-both-chain-withdraw-impossible-collections-array-will-be-OOG

Likelyhood: High, once the whitelist option is disabled, collections will grow. Impact: High, withdraw won’t be possible because of Out-Of-Gas.

Support

FAQs

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