NFTBridge
60,000 USDC
View results
Submission Details
Severity: medium
Valid

When the user calls `bridge.cairo::deposit_tokens()` with `use_withdraw_auto = true`, the bridged `NFT` will be locked in the bridge contract.

Summary

When the user calls bridge.cairo::deposit_tokens() with use_withdraw_auto = true, the bridged NFT will be locked in the bridge contract.

Vulnerability Details

The user calls the bridge.cairo::deposit_tokens() function in the L2 contract and sets use_withdraw_auto to true to perform cross-chain operations.

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);
// SNIP..
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(),
};
// SNIP...
}
// request.cairo::compute_request_header_v1()
fn compute_request_header_v1(
ctype: CollectionType,
use_deposit_burn_auto: bool,
@> use_withdraw_auto: bool,
) -> felt252 {
let mut header: u256 = HEADER_V1;
// SNIP...
if (use_withdraw_auto) {
@> header = header | WITHDRAW_AUTO;
}
header.try_into().expect('Invalid header V1')
}

However, when the user calls the Starklane::withdrawTokens() method in the L1 contract to perform the withdrawal, the program will revert due to Protocol.canUseWithdrawAuto(header) == true, and the user's transferred NFT will be locked in the bridge contract.

function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
if (!_enabled) {
revert BridgeNotEnabledError();
}
// Header is always the first uint256 of the serialized request.
uint256 header = request[0];
// Any error or permission fail in the message consumption will cause a revert.
// After message being consumed, it is considered legit and tokens can be withdrawn.
@> if (Protocol.canUseWithdrawAuto(header)) {
// 2024-03-19: disabled autoWithdraw after audit report
// _consumeMessageAutoWithdraw(_starklaneL2Address, request);
@> revert NotSupportedYetError();
} else {
_consumeMessageStarknet(_starknetCoreAddress, _starklaneL2Address, request);
}
// SNIP...
}
// Protocol::canUseWithdrawAuto()
function canUseWithdrawAuto(
uint256 header
)
internal
pure
returns (bool)
{
return (header & WITHDRAW_AUTO) == WITHDRAW_AUTO;
}

Code Snippet

https://github.com/ArkProjectNFTs/bridge/blob/1bb58731d8e4c37a71d3611c8ea6163c9b019193/apps/blockchain/starknet/src/bridge.cairo#L242-L306
https://github.com/ArkProjectNFTs/bridge/blob/1bb58731d8e4c37a71d3611c8ea6163c9b019193/apps/blockchain/starknet/src/request.cairo#L112-L136
https://github.com/ArkProjectNFTs/bridge/blob/1bb58731d8e4c37a71d3611c8ea6163c9b019193/apps/blockchain/ethereum/src/Bridge.sol#L153-L215
https://github.com/ArkProjectNFTs/bridge/blob/1bb58731d8e4c37a71d3611c8ea6163c9b019193/apps/blockchain/ethereum/src/Protocol.sol#L57-L65

Impact

When the user calls bridge.cairo::deposit_tokens() with use_withdraw_auto = true, the bridged NFT will be locked in the bridge contract.

Tools Used

Manual Review

Recommendations

It is recommended to modify the bridge.cairo::deposit_tokens() method to force use_withdraw_auto to be set to false.

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);
// SNIP..
let req = Request {
- header: compute_request_header_v1(ctype, use_deposit_burn_auto, use_withdraw_auto),
+ header: compute_request_header_v1(ctype, use_deposit_burn_auto, false),
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(),
};
// SNIP...
}
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.