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

There is no way to upgrade NFTs collections on `L1` that is mapped to Original Collections on `L2`

alqaqa

Vulnerability Details

When we are bridging Tokens L1<->L2 if the NFT collection in the source chain has no address on the destination chain, we are deploying new ERC721 NFT collection addresses and attaching them.

When deploying we are making the Collection upgradable, where we can change the implementation of that NFT collection if needed.

Deployer.sol#L23-L38

function deployERC721Bridgeable(... ) ... {
❌️ address impl = address(new ERC721Bridgeable());
bytes memory dataInit = abi.encodeWithSelector(
ERC721Bridgeable.initialize.selector,
abi.encode(name, symbol)
);
​
❌️ return address(new ERC1967Proxy(impl, dataInit));
}

Since ERC1967 makes the Proxy admin is the sender, so the Bridge is the only address that can upgrade the Collection implementation.

The issue is that no method exists in our L1Bridge contract to upgrade the NFT collection implementation in L1. However, if we checked the L2Bridge we will find that upgrading an NFT collection is a supported thing and intended.

bridge.cairo#L369-L373

fn collection_upgrade(ref self: ContractState, collection: ContractAddress, class_hash: ClassHash) {
ensure_is_admin(@self);
IUpgradeableDispatcher { contract_address: collection }
.upgrade(class_hash);
}

As we can see in L2Bridge there is a function collection_upgrade which takes the NFT collection address and calls upgrade, which will allow upgrading the created NFT collection implementation if needed.

This will result in the inability to upgrade NFT collections that was deployed on L1 if needed.

Proof of Concept

  • Bridge is active

  • Tokens are Bridge L1<->L2

  • There are new collections created on L1 and attached to original collections on L2

  • There are new collections created on L2 and attached to original collections on L1

  • The admin decided to upgrade some collections created on L1 and L2

  • The admin will be able to upgrade L2 collections but will not be able to upgrade L1 collections

Impact

Inability to upgrade Created NFT collections on L1

Tools Used

Manual Review

Recommendation

Implement a function to upgrade the NFT collection on L1Bridge, the same as that in L2Bridge.

diff --git a/apps/blockchain/ethereum/src/Bridge.sol b/apps/blockchain/ethereum/src/Bridge.sol
index e62c7ce..0df98c2 100644
--- a/apps/blockchain/ethereum/src/Bridge.sol
index e62c7ce..0df98c2 100644
--- a/apps/blockchain/ethereum/src/Bridge.sol
+++ b/apps/blockchain/ethereum/src/Bridge.sol
@@ -374,4 +374,16 @@ contract Starklane is IStarklaneEvent, UUPSOwnableProxied, StarklaneState, Stark
emit L1L2CollectionMappingUpdated(collectionL1, snaddress.unwrap(collectionL2));
}
+ function collectionUpgrade(
+ ERC721Bridgeable collectionAddress,
+ address newImplementation,
+ bytes memory initData
+ ) external onlyOwner {
+ if (init.length > 0) {
+ collectionAddress.upgradeToAndCall(newImplementation, initData);
+ } else {
+ collectionAddress.upgradeTo(newImplementation);
+ }
+ }
+
}

NOTE: This Mitigation is not tested, so it may be implemented correctly.

Updates

Lead Judging Commences

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

finding-no-transferOwnership-or-upgrade-for-collections-in-CollectionManager

Likelyhood/Impact: High, it will never (until an upgrade) be able to update or transfer the ownership of any collections created on L1.

Support

FAQs

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