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

An attacker could repeatedly bridge L2 NFTs to L1, potentially triggering an out-of-gas DoS attack on the `withdrawTokens` functionality

said

Summary

An attacker could continuously bridge L2 NFTs to L1, causing the _collections array to grow excessively and potentially leading to an out-of-gas DoS attack when users calling withdrawTokens.

Vulnerability Details

When users bridge an NFT collection from L2 to L1 and the corresponding L1 collection does not exist, it will be created, added to the whitelist, and included in the _collections array by calling _whiteListCollection.

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

function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
// ...
​
address collectionL1 = _verifyRequestAddresses(req.collectionL1, req.collectionL2);
​
CollectionType ctype = Protocol.collectionTypeFromHeader(header);
​
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();
}
}
​
// ...
}

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

function _whiteListCollection(address collection, bool enable) internal {
if (enable && !_whiteList[collection]) {
bool toAdd = true;
uint256 i = 0;
while(i < _collections.length) {
if (collection == _collections[i]) {
toAdd = false;
break;
}
i++;
}
if (toAdd) {
>>> _collections.push(collection);
}
}
_whiteList[collection] = enable;
}

If the whitelist functionality is disabled on L2, an attacker could repeatedly call deposit_tokens with newly created NFT collections. Additionally, because there is no minimum token_ids check, the attacker can provide empty token_ids to make the attack even cheaper.

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

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 collection_l1 = self.l2_to_l1_addresses.read(collection_l2);
​
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
});
}

This scenario could also eventually occur without any malicious intent if the whitelist is disabled and enough different collections are added on L1, leading to an out-of-gas situation and potentially bricking the withdrawTokens functionality.

Impact

the withdrawTokens functionality will be DoSed.

Tools Used

Manual review

Recommendations

Remove the addition of newly created collections to _collections within with drawTokens to avoid this situation, and add functionality to remove collections from _collections.

Updates

Lead Judging Commences

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