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

Adding the `from` parameter to the `bridge.cairo::deposit_tokens()` function may result in the theft of NFTs.

joicygiore

Summary

Adding the from parameter to the bridge.cairo::deposit_tokens() function may result in the theft of NFTs.

Vulnerability Details

The source code of bridge.cairo::deposit_tokens() is shown below, with emphasis on the // TODO: part. Currently, from is set to the caller’s address. However, if from is changed to a parameter, it means that as long as the from address has authorized the bridge.cairo contract to use the NFT, anyone could transfer the NFT to any owner_l1, leading to potential theft.

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();
​
//SNIP...
}

Consider the following scenario:

  1. User A authorizes the bridge.cairo contract to use their NFT.

  2. After User A completes the authorization, Operator B calls bridge.cairo::deposit_tokens() and uses the from parameter to help User A complete the cross-chain operation.
    (Note: Once User A's authorization transaction is included in a block and successfully submitted to the blockchain, all transaction details (including the authorized NFT and contract address) become public. The transparency of the blockchain ensures that anyone can view this authorization transaction.)

  3. If Operator B fails to act promptly, a malicious User C could call bridge.cairo::deposit_tokens() to cross-chain the NFT to their own owner_l1.

Code Snippet

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

Impact

If the from parameter is added to the bridge.cairo::deposit_tokens() function, it could lead to the theft of NFTs.

Tools Used

Manual Review

Recommendations

It is recommended not to add the from parameter or similar implementations, and to ensure that the caller's address is always used as from.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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