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

Unused `useAutoBurn` and `use_deposit_burn_auto` parameters in deposit calls on L1/L2 can cause confusion over NFT burn status

hydrapenta

Summary

The useAutoBurn parameter in the depositTokens function on L1 and the use_deposit_burn_auto parameter in the deposit_tokens function on L2 are both designed to control the automatic burning of tokens during L1/L2 deposit calls. However, these parameters are currently not utilized, potentially leading to user confusion as they may mistakenly believe their NFTs are being burned when they are not.

Vulnerability Details

In the depositTokens function on L1, the useAutoBurn parameter is intended to control whether tokens are automatically burned on L1 instead of being sent to escrow. Despite this, the parameter is not utilized in the function, meaning that the tokens are not automatically burned, even if the user expects them to be.

function depositTokens(
address l2Recipient,
uint256 amount,
bool useAutoBurn
) external payable {
// Parameter `useAutoBurn` is passed but never used
...
}
​

The deposit_tokens function on L2 includes the use_deposit_burn_auto parameter, which is meant to trigger the automatic burning of tokens on L2 after transfer via the Starklane indexer. However, this parameter is also unused in the function, leading to the same issues as on L1, where users might incorrectly believe their tokens are being burned.

/// Deposits tokens to be bridged on the L1.
///
/// # Arguments
///
/// * `salt` - Randome salt to compute request hash.
/// * `collection_l2` - Address of the collection on L2.
/// * `owner_l1` - Address of the owner on L1.
/// * `tokens_ids` - Tokens to be bridged on L1.
/// * `use_withdraw_auto` - Tokens are automatically withdrawn on L1 using Starklane indexer.
/// * `use_deposit_burn_auto` - Tokens are automatically burnt on L2 once transferred using Starklane indexer.
///
/// TODO: add new_owners, values and uris.
/// TODO: better to use a struct than too much arguments? (DepositParams/DepositInputs/???)
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, //@audit this is not implemented -> tokens should be burnt instyead of
) {
}

Impact

Users may mistakenly believe that their NFTs are being automatically burned on either L1 or L2 when they are not, leading to confusion and potential operational errors.

Tools Used

Manual Review.

Recommendations

If the implementation is expected to be built in future versions, consider preventing the user from choosing the auto burn feature.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Informational / Gas

Please, do not suppose impacts, think about the real impact of the bug and check the CodeHawks documentation to confirm: https://docs.codehawks.com/hawks-auditors/how-to-determine-a-finding-validity A PoC always helps to understand the real impact possible.

Support

FAQs

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