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

Existed collections are not whitelisted when Bridging

alqaqa

Vulnerability Details

When Bridging NFTs between L1<->L2, we are whitelisting the NFT collection in the destination chain if it is not whitelisted, and since we are checking that depositing NFTs from the source chain should be from a whitelisted NFT, it is important to integrability between L1 and L2.

The problem is that we are only whitelisting the collection on the destination chain if it doesn't have an attachment to it on L1, whereas after deploying it we are whitelisting it. But if the collection already existed we are not whitelisting it if needed.

Bridge.sol#L183-L196

if (collectionL1 == address(0x0)) {
if (ctype == CollectionType.ERC721) {
collectionL1 = _deployERC721Bridgeable( ... );
// update whitelist if needed
❌️ _whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}

As we can see, whitelisting occurs only if the NFT collection does not exist on L1, so whitelisting the collection will not occur if the collection already exists.

We will discuss the scenario where this can occur and introduce problems in the Proof of Concept section.

Proof of Concept

  • whitelisting is disabled, and any NFT can be Bridged.

  • NFTs are Bridged from L1, To L2.

  • NFT collections on L1 are not whitelisted collections now (whitelisting is disabled).

  • On L2 we are deploying addresses that maps to Collections on L1 and whitelist them.

  • the protocol enables whiteListing.

  • NFTs collections that were created on L2 are now whitelisted and can be Bridged to L1, but still, these Collection L1 addresses are not whitelisted.

  • We can Bridge from L2 to L1 easily as these collections are whitelisted.

  • We can withdraw the NFT from L1 easily (there is no check for whitelisting when withdrawing).

  • since L1<->L2 mapping is already set. NFTs Collections are still not-whitelisted on L1.

  • The process will end up with the ability to Bridge NFTs from L2 to L1 as they are whitelisted on L2, but the inability to Bridge them from L1 to L2 as they are not whitelisted on L1.

Tools Used

Manual Review

Recommendations

Whitelist the collection if it is newly deployed or if it already exists and is not whitelisted.

diff --git a/apps/blockchain/ethereum/src/Bridge.sol b/apps/blockchain/ethereum/src/Bridge.sol
index e62c7ce..e069544 100644
--- a/apps/blockchain/ethereum/src/Bridge.sol
+++ b/apps/blockchain/ethereum/src/Bridge.sol
@@ -188,13 +188,14 @@ contract Starklane is IStarklaneEvent, UUPSOwnableProxied, StarklaneState, Stark
req.collectionL2,
req.hash
);
- // update whitelist if needed
- _whiteListCollection(collectionL1, true);
} else {
revert NotSupportedYetError();
}
}
+ // update whitelist if needed
+ _whiteListCollection(collectionL1, true);
+
for (uint256 i = 0; i < req.tokenIds.length; i++) {
uint256 id = req.tokenIds[i];

Existence of the issue on the Starknet Side

This issue explains the flow from Ethereum to Starknet. the problem also existed in L2 side, where we are only whitelisting if there is no address for that collection on L2. If the collection_l2 == 0, we are deploying the collection on L2 and whitelist it. But if it is already existed we are just returning the address without whitelisting it if existed, same as that in L1.

bridge.cairo#L440-L442

fn ensure_erc721_deployment(ref self: ContractState, req: @Request) -> ContractAddress {
...
let collection_l2 = verify_collection_address( ... );
​
if !collection_l2.is_zero() {
❌️ return collection_l2;
}
...
// update whitelist if needed
let (already_white_listed, _) = self.white_listed_list.read(l2_addr_from_deploy);
if already_white_listed != true {
_white_list_collection(ref self, l2_addr_from_deploy, true);
...
}
l2_addr_from_deploy
}

So if we changed the order, the Collections can be whitelisted on L1, but not on L2 (if the Proof of Concept was in the reverse order), Bridging these NFTs collections from L1 to L2 will be allowed, but we will not be able to Bridge them from L2 to L1, as they are not whitelisted on L2.

Recommendations: the same as that in L1, we need to whitelist collections if they already exist.

diff --git a/apps/blockchain/starknet/src/bridge.cairo b/apps/blockchain/starknet/src/bridge.cairo
index 23cbf8a..1d3521e 100644
--- a/apps/blockchain/starknet/src/bridge.cairo
+++ b/apps/blockchain/starknet/src/bridge.cairo
@@ -438,6 +438,15 @@ mod bridge {
);
if !collection_l2.is_zero() {
+ // update whitelist if needed
+ let (already_white_listed, _) = self.white_listed_list.read(l2_addr_from_deploy);
+ if already_white_listed != true {
+ _white_list_collection(ref self, l2_addr_from_deploy, true);
+ self.emit(CollectionWhiteListUpdated {
+ collection: l2_addr_from_deploy,
+ enabled: true,
+ });
+ }
return collection_l2;
}

NOTE: if the issue will be judged as two separate issues. Please, consider making this report as duplicate of both of them.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

finding-reactivate-whitelist-can-prevent-bridging-whitelisted-tokens

Impact: If the whitelist option is disable and a collection has only bridged NFTs from one side before enabling the whitelist option again, one side won’t be able to bridge a whitelisted token. Likelyhood: Low, Will be rare and unexpected but can happen.

Appeal created

alqaqa Submitter
9 months ago
n0kto Lead Judge
8 months ago
n0kto Lead Judge 8 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-reactivate-whitelist-can-prevent-bridging-whitelisted-tokens

Impact: If the whitelist option is disable and a collection has only bridged NFTs from one side before enabling the whitelist option again, one side won’t be able to bridge a whitelisted token. Likelyhood: Low, Will be rare and unexpected but can happen.

Support

FAQs

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