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

If whitelist is off someone can add a lot of collections in the whitelist, DOSing the creation and whitelisting of legitimate collections

0xasen

Summary

If whitelist is off, someone can add a lot of collections to the whitelist DOSing the creation and whitelisting of legitimate collections. The DOS would occur in this function in Bridge.sol:

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;
}

Vulnerability Details

When the whitelist is off on any of the bridges, anyone can add whatever collection they like by bridging it from L1 -> L2 or vice versa. Let's take a look at this part of the Bridge::withdrawToken() function:

function withdrawTokens(
uint256[] calldata request
)
external
payable
returns (address)
{
if (!_enabled) {
revert BridgeNotEnabledError();
}
​
uint256 header = request[0];
​
if (Protocol.canUseWithdrawAuto(header)) {
revert NotSupportedYetError();
} else {
_consumeMessageStarknet(_starknetCoreAddress, _starklaneL2Address, request);
}
​
Request memory req = Protocol.requestDeserialize(request, 0);
​
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
_whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}
​

As you can see when the collection is not deployed it'll be automatically added to the whitelist. Now let's take a closer look at the _whiteListCollection function:

function _whiteListCollection(address collection, bool enable) internal {
​
if (enable && !_whiteList[collection]) {
bool toAdd = true;
uint256 i = 0;
// loop through all of the collections
while(i < _collections.length)
{
if (collection == _collections[i]) {
toAdd = false;
break;
}
i++;
}
if (toAdd) {
//push the collection to the array
_collections.push(collection);
}
}
​
_whiteList[collection] = enable;
}

As you can see, the function loops through all of the collections no matter if they are whitelisted or not.

So what could happen is that when the whitelist is off, a malicious actor can create a very big number of collections, in our example on L2, then bridge them to L1, which adds them both to the whitelist and to the collections array, causing the _whiteListCollection function to revert due to an Out-of-Gas error.

This will DOS the whitelisting of collections and by extension DOS on the withdrawTokens() function which would lead to legitimate users' tokens being stuck permanently in the contract. In our example, the tokens will be stuck in the L2 contract and won't be withdrawable on the L1 contract.

The impact and the possibility of this attack are increased since both sides of the bridge are deployed with the whitelist being off by default:

bool _whiteListEnabled;
​
function initialize(
bytes calldata data
)
public
onlyInit
{
(
address owner,
IStarknetMessaging starknetCoreAddress,
uint256 starklaneL2Address,
uint256 starklaneL2Selector
) = abi.decode(
data,
(address, IStarknetMessaging, uint256, uint256)
);
_enabled = false;
_starknetCoreAddress = starknetCoreAddress;
​
_transferOwnership(owner);
​
setStarklaneL2Address(starklaneL2Address);
setStarklaneL2Selector(starklaneL2Selector);
}

The _whiteListEnabled bool is not set during initialization and bool's default value is false. And on the L2 side:

fn constructor(
ref self: ContractState,
bridge_admin: ContractAddress,
bridge_l1_address: EthAddress,
erc721_bridgeable_class: ClassHash,
) {
self.ownable.initializer(bridge_admin);
self.bridge_l1_address.write(bridge_l1_address);
self.erc721_bridgeable_class.write(erc721_bridgeable_class);
self.white_list_enabled.write(false);
​
self.enabled.write(false); // disabled by default <---
}

This opens up the possibility that a malicious actor could perform the attack in between deployment and enabling the whitelist but it's also possible to be executed each time the whitelist is being off.

Once the attack is performed the bridge would be unusable by any new collections since there is no mechanism to remove collections from the collections array.

Impact

Users' tokens will get stuck permanently in the bridge contracts.

Tools Used

Manual review

Recommendations

Find a more efficient way to loop through the collections array or remove the array entirely. We're only interested in the whitelisted collection anyway.
If you decide to stick with the collections array, add a function that allows an admin to remove elements from it.
I'd also suggest that the bridges should be deployed with an enabled whitelist by default.

Updates

Lead Judging Commences

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