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

Locked tokens in L2 due to revert of withdrawTokens

ge6a

Summary

When sending tokens from L2 to L1, the tokens are locked in escrow on L2, and the user can receive the corresponding tokens on L1 by calling the withdrawTokens function. If use_withdraw_auto=true is provided, the function will revert. This is intentional and was done as a result of an audit, as mentioned in the comment. However, nothing prevents the L2 user from setting this flag to true. As a result, tokens will not be received on L1, and the tokens on L2 will remain locked.

Vulnerability Details

Let's look at the deposit_tokens function in the bridge.cairo file. It is used for sending tokens from L2 to L1.

fn deposit_tokens(
ref self: ContractState,
salt: felt252,
collection_l2: ContractAddress,
owner_l1: EthAddress,
token_ids: Span<u256>,
@> use_withdraw_auto: bool,
use_deposit_burn_auto: bool,
) {
ensure_is_enabled(@self);
assert(!self.bridge_l1_address.read().is_zero(), 'Bridge is not open');
​
​
// TODO: we may have the "from" into the params, to allow an operator
// to deposit token for a user.
// This can open more possibilities.
// Because anyway, a token can't be moved if the owner of the token
// is not giving approval to the bridge.
let from = starknet::get_caller_address();
​
​
assert(_is_white_listed(@self, collection_l2), 'Collection not whitelisted');
​
​
// TODO: add support for 1155 and check the detected interface on the collection.
let ctype = CollectionType::ERC721;
​
​
let erc721_metadata = erc721_metadata(collection_l2, Option::Some(token_ids));
let (name, symbol, base_uri, uris) = match erc721_metadata {
Option::Some(data) => (data.name, data.symbol, data.base_uri, data.uris),
Option::None => ("", "", "", array![].span())
};
​
​
escrow_deposit_tokens(ref self, collection_l2, from, token_ids);
​
​
let collection_l1 = self.l2_to_l1_addresses.read(collection_l2);
​
​
let req = Request {
@> header: compute_request_header_v1(ctype, use_deposit_burn_auto, use_withdraw_auto),

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

From the provided code, it is clear that use_withdraw_auto is passed by the user, stored in the request header, and nowhere is its value checked to see if it is true. The function is accompanied by the following comment:

use_withdraw_auto - Tokens are automatically withdrawn on L1 using Starklane indexer.

which gives the impression that both true and false values are possible. The user has no way of knowing that setting it to true will lead to their tokens being locked in escrow on L2 without receiving the corresponding tokens on L1. In the L2->L1 direction there is no cancel message functionality.

Impact

Lock of funds for the user and broken functionality.

Tools Used

Manual review

Recommendations

Udjust the deposit_tokens function to always pass use_withdraw_auto=false.

Updates

Lead Judging Commences

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

finding-auto_withdrawn-L2-NFT-stuck

Impact: High, token will be stuck in L2 bridge. Likelyhood: Very low, option is available in L2 but has been disabled since March on L1, would be almost a user error.

Support

FAQs

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