ArkProject: NFT Bridge

Summary

Calls to Starklane::withdrawTokens that require deploying a new ERC721Bridgable, also require whitelisting the contract, which traverses the entire _collections array. Since the collections array includes any contract that was ever whitelisted (collections are not removed when they are blacklisted) it is grow-only and will eventually cause gas cost to be prohibitive/exceed block limit.

Vulnerability Details

When Starklane::withdrawTokens is called on Ethereum for a collection that is bridged for the first time, a new ERC721Bridgable contract is deployed and then added to the whitelist. Whitelisting the contract involved traversing the entire _collections array (that keeps any contract that was ever whitelisted):

The _collections array contains every collection that was ever whitelisted (collections are not removed when blacklisted), making the gas cost of these calls ever-growing until it reaches a prohibitive cost and eventually breach the block gas limit.

The estimated cost of a storage read is ~2500 gas units. The block gas limit on Ethereum is 30M. The current gas requried for WithdrawToken with deploy is ~2,000,000 (based on Foundry tests). This means that with roughly 11,000 collections whitelisted on the bridge, calling withdrawTokens for new collections will become impossible (likely the dollar cost will become prohibitive long before that).

Impact

1. DOS of L1 withdrawals (either due to prohibitve cost or block gas limit)

2. Potential permanent lock of tokens on L2 as a result of 1.

Tools Used

Manual Review, Foundry

Recommended Mitigation

1. Change the _whiteList map to a mapping of address => struct(bool,bool) where the second boolean marks weather or not the collection is already in the _collections array (making the traversal of _collections in _whiteListCollection() redundant).

2. Alternatively use something like openzeppelin EnumerableMap (which handles Set, Remove efficiently)