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

Critical Overconstrained Check in L1-L2 Interaction: Enforcing use_withdraw_auto to Prevent Transaction Reversion

0xgenaudits

Summary

Check this security consideration from Trail of Bits: https://github.com/crytic/building-secure-contracts/tree/master/not-so-smart-contracts/cairo/overconstrained_l1_l2_interaction

Make sure to validate that the checks on both the L1 and L2 side are similar enough to prevent unexpected behavior. Ensure that any unsymmetric validations on either side cannot lead to a tokens being trapped or any other denial of service.

The sponsor in video says auditors should focus on I quote:

"auditors should focus on serveral critical aspects first is security of the NFTs Locking Unlocking"

This link to video: https://youtu.be/vZhI-hiA_hI?t=983

A critical issue has been identified in the interaction between L1 and L2 when users set use_withdraw_auto to true.
This configuration leads to transaction reversion on L1 due to an overconstrained check within the protocol Protocol.canUseWithdrawAuto(header)

Vulnerability Details

The vulnerability stems from the overconstrained logic applied during L1 and L2 interactions, specifically when the use_withdraw_auto
flag is set to true. The Protocol.canUseWithdrawAuto(header)
function contains a check that, when the flag is set to true, leads to transaction failure on L1. resulting in failed transactions and potentially locking assets in the protocol.

POC:

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,
) {
// ...
let req = Request {
@>>> // use_withdraw_auto is passed directly.
@>>> header: compute_request_header_v1(ctype, use_deposit_burn_auto, use_withdraw_auto),
hash: compute_request_hash(salt, collection_l2, owner_l1, token_ids),
//...
};
// ...
}
function withdrawTokens(uint256[] calldata request) external payable returns(address) {
//...
@>>> if (Protocol.canUseWithdrawAuto(header)) {
// 2024-03-19: disabled autoWithdraw after audit report
// _consumeMessageAutoWithdraw(_starklaneL2Address, request);
revert NotSupportedYetError();
} else {
_consumeMessageStarknet(_starknetCoreAddress, _starklaneL2Address, request);
}
//...
}

Impact

If the use_withdraw_auto flag is set to true, the transaction will revert on L1 due to if (Protocol.canUseWithdrawAuto(header))
This reversion preventing them from successfully withdrawing their assets and potentially causing losses.

Recommendations

To mitigate this issue, users should be enforced to set use_withdraw_auto to false in their transactions.

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,
) {
// ...
let req = Request {
// use_withdraw_auto is passed directly.
- 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),
//...
};
// ...
}
​
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year 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.