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

If the recipient address is blacklisted by the `collectionL1` or `collectionL2` contract, the `NFT` will be permanently locked in the bridge contract.

joicygiore

Summary

If the recipient address is blacklisted by the collectionL1 or collectionL2 contract, the NFT will be permanently locked in the bridge contract.

Vulnerability Details

Let's take L2->L1 as an example:

The user calls the bridge.cairo::deposit_tokens() function to bridge an NFT from L2 back to L1. This method calls the core function starknet::send_message_to_l1_syscall() to complete the L2->L1 information transmission. At this point, the owner_l1 address has been determined.

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');
​
// 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(),
};
​
​
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
});
}

After the data is sent, the user will call the Starklane::withdrawTokens() function in the L1 contract, transferring the NFT to req.ownerL1 through the StarklaneEscrow::_withdrawFromEscrow() function, thus completing the cross-chain process. However, if the req.ownerL1 address is blacklisted by the collectionL1 contract, the transfer will fail, and the NFT will be permanently locked in the bridge contract.

// Starklane::withdrawTokens()
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...
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
​
​
@> bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
​
// SNIP...
}
// StarklaneEscrow::_withdrawFromEscrow()
function _withdrawFromEscrow(
CollectionType collectionType,
address collection,
address to,
uint256 id
)
internal
returns (bool)
{
if (!_isEscrowed(collection, id)) {
return false;
}
​
​
address from = address(this);
​
​
if (collectionType == CollectionType.ERC721) {
@> IERC721(collection).safeTransferFrom(from, to, id);
} else {
// TODO:
// Check here if the token supply is currently 0.
IERC1155(collection).safeTransferFrom(from, to, id, 1, "");
}
​
​
_escrow[collection][id] = address(0x0);
​
​
return true;
}

Code Snippet

https://github.com/Cyfrin/2024-07-ark-project/blob/273b7b94986d3914d5ee737c99a59ec8728b1517/apps/blockchain/starknet/src/bridge.cairo#L129-L181
https://github.com/ArkProjectNFTs/bridge/blob/1bb58731d8e4c37a71d3611c8ea6163c9b019193/apps/blockchain/ethereum/src/Bridge.sol#L153-L215
https://github.com/ArkProjectNFTs/bridge/blob/1bb58731d8e4c37a71d3611c8ea6163c9b019193/apps/blockchain/starknet/src/bridge.cairo#L242-L306
https://github.com/ArkProjectNFTs/bridge/blob/1bb58731d8e4c37a71d3611c8ea6163c9b019193/apps/blockchain/ethereum/src/Escrow.sol#L63-L89

Impact

If the recipient address is blacklisted by the collectionL1 or collectionL2 contract, the NFT will be permanently locked in the bridge contract.

Tools Used

Manual Review

Recommendations

If the recipient address is in the blacklist of collectionL1, there is currently no direct remedy. To address this issue, the following strategies can be considered:

  1. Provide Address Update Function:
    Add a function in the L2 contract to allow users to change the recipient address before or after initiating a cross-chain transaction on the L2 side. This requires close cooperation between L1 and L2 contracts.

  2. Administrator Intervention:
    In extreme cases, allow administrators to manually change the recipient address in the L1 contract or perform other necessary operations to ensure that NFT can be extracted smoothly.

Note: L2->L1 should also add corresponding implementation according to the same logic

Updates

Lead Judging Commences

n0kto Lead Judge 10 months 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.

Appeal created

joicygiore Submitter
10 months ago
n0kto Lead Judge
10 months ago
n0kto Lead Judge 9 months 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.