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

No maximum token amount check on `deposit_tokens` on L2 may cause tokens to get stuck on L1.

said

Summary

Due to the lack of a maximum token amount check on deposit_tokens on L2, withdrawTokens on L1 could revert if it exceeds the gas limit.

Vulnerability Details

A maximum token amount check is crucial. If users provide too many tokens, they might not be able to claim the NFTs on the destination chain due to gas limits. This check exists on L1 and, while it doesn’t directly limit the number of tokens, it ensures the serialized payload doesn’t exceed MAX_PAYLOAD_LENGTH, thereby implicitly limiting the amount of tokens sent to L2.

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L134-L136

function depositTokens(
uint256 salt,
address collectionL1,
snaddress ownerL2,
uint256[] calldata ids,
bool useAutoBurn
)
external
payable
{
// ...
>>> if (payload.length >= MAX_PAYLOAD_LENGTH) {
revert TooManyTokensError();
}
IStarknetMessaging(_starknetCoreAddress).sendMessageToL2{value: msg.value}(
snaddress.unwrap(_starklaneL2Address),
felt252.unwrap(_starklaneL2Selector),
payload
);
​
emit DepositRequestInitiated(req.hash, block.timestamp, payload);
}

However, a similar check doesn't exist on L2, allowing users to send any amount of NFTs to L1.

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

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),
hash: compute_request_hash(salt, collection_l2, owner_l1, token_ids),
collection_l1,
collection_l2,
owner_l1,
owner_l2: from,
name,
symbol,
base_uri,
ids: token_ids,
values: array![].span(),
uris,
new_owners: array![].span(),
};
​
let mut buf: Array<felt252> = array![];
req.serialize(ref buf);
​
starknet::send_message_to_l1_syscall(
self.bridge_l1_address.read().into(),
buf.span(),
)
.unwrap_syscall();
self.emit(DepositRequestInitiated {
hash: req.hash,
block_timestamp: starknet::info::get_block_timestamp(),
req_content: req
});
}

Impact

This could result in a scenario where users cannot claim the tokens on L1 due to exceeding the gas limit.

Tools Used

Manual review

Recommendations

Consider adding a maximum token amount limit when users call deposit_tokens on L2, and implementing the same check on L1 when triggering depositTokens, in addition to the existing MAX_PAYLOAD_LENGTH check.

Updates

Lead Judging Commences

n0kto Lead Judge 11 months ago
Submission Judgement Published
Invalidated
Reason: Known issue

Support

FAQs

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