Summary

In the withdrawTokens function, if the collectionL1 parameter is set to address(0), an ERC721Bridgeable contract is deployed, and the resulting address is automatically whitelisted. This logic does not consider the situation when _whiteListEnabled == false (i.e., when the whitelist mechanism is disabled), which leads to unintended tokens being whitelisted.

If _whiteListEnabled is later changed to true to restrict token bridging, the previously whitelisted tokens will remain whitelisted, potentially enabling malicious or unintended tokens to be bridged without restriction. Additionally, adding a large number of tokens to the _collections array could cause Out-of-gas issues due to excessive iteration during whitelist management.

Vulnerability Details

The _whiteListEnabled and white_list_enabled variables on Ethereum and Starknet, respectively, indicate whether the whitelist mechanism is active.

If these variables are false, the whitelist check is bypassed, and all tokens are allowed to bridge across chains by default:

It is pretty sure that _whiteListEnabled or white_list_enabled should be set to be the same to apply same criteria for consistent cross-chain operations, otherwise the uni-directional cross-chain would cause tokens to be stuck.

The problem arises in the withdrawTokens function, where if collectionL1 == address(0), a new ERC721Bridgeable contract is deployed, and the resulting address is automatically added to the whitelist:

This is the same in bridge.cairo:

This logic automatically adds the newly deployed collection to the whitelist regardless of the _whiteListEnabled or white_list_enabled values. As a result, even if _whiteListEnabled is later changed back to true, these tokens will remain in the whitelist, which is unintended.

Consider a following scenario:

1. The _whiteListEnabled on Ethereum and white_list_enabled on Starknet are both set to false. This allows any token to be bridged without restriction. A malicious token is bridged from Starknet to Ethereum. Since the whitelist is disabled, no checks are performed, and the token is accepted.

2. During the withdrawTokens process, if collectionL1 == address(0), an ERC721Bridgeable contract is deployed on Ethereum. This new collectionL1 address is automatically whitelisted, regardless of the _whiteListEnabled status. The malicious token’s collection is now part of the whitelist unintentionally.

3. Later, due to security concerns, the contract owner re-enables the whitelist mechanism by setting _whiteListEnabled = true on Ethereum and white_list_enabled = true on Starknet. However, since the malicious token’s collection was already added to the whitelist, it remains whitelisted even with the whitelist mechanism now enabled.

4. The malicious token can still be bridged back because it remains on the whitelist. This undermines the security purpose of re-enabling the whitelist.

Consider another scenario:

1. The _whiteListEnabled on Ethereum and white_list_enabled on Starknet are both set to false. This allows any token to be bridged without restriction.

2. A malicious actor bridges 1,000 different tokens from Starknet to Ethereum. Since the whitelist is disabled, all tokens are accepted without any checks.

3. During the withdrawTokens process, for each of the 1,000 tokens, if collectionL1 == address(0), an ERC721Bridgeable contract is deployed on Ethereum for each token. Each of these 1,000 new collectionL1 addresses is automatically whitelisted, regardless of the _whiteListEnabled status.

4. The whitelist array _collections now contains 1,000 new entries, which were automatically added due to the disabled whitelist mechanism. This massive addition of collections dilutes the whitelist array, making it cumbersome and inefficient to manage.

5. The _collections array is iterated over in functions like _whiteListCollection. With the sudden influx of 1,000 new entries, these iterations become costly in terms of gas. As the number of entries grows, operations that involve iterating over the whitelist could run into out-of-gas errors, causing critical functions to fail.
   
   A brief PoC is shown below:

It is confirmed that the collection is still whitelisted after _whiteListEnabled = true.

Impact

1. Unintended Token Whitelisting: Tokens that should not be whitelisted may remain whitelisted, even after the whitelist mechanism is re-enabled, leading to potential security risks.

2. Out-of-Gas Issues: Excessive additions to the _collections array could cause out-of-gas issues during whitelist management, particularly when iterating through the _collections array in the _whiteListCollection function.

Tools Used

Manual

Recommendations

To mitigate this issue, it is recommended to check the status of _whiteListEnabled and white_list_enabled before adding deployed tokens to the whitelist. When _whiteListEnabled == false, tokens should not be automatically added to the whitelist.