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

There is no way to change NFTs collections Ownership 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 the NFT collection, we are deploying it with an owner, which is L1Bridge.

Deployer.sol#L32-L35

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));
}
​
...
​
function initialize( ... ) ... {
...
❌️ _transferOwnership(_msgSender());
}

Ownership is transferred to the msg.sender of initialize, which is L1Bridge who deployed the proxy contract with that init data.

The problem is there is no way to transfer the ownership of a given ERC721 collection, which is not the case in L2, where the ownership can be transferred to another owner.

bridge.cairo#L375-L380

// transfer owner of the given collection to the given address
fn collection_transfer_ownership(ref self: ContractState, collection: ContractAddress, new_owner: ContractAddress) {
ensure_is_admin(@self);
IOwnableDispatcher { contract_address: collection }
.transfer_ownership(new_owner);
}

This will result inability to transfer the ownership of a given collection on L2 that has a deployed address on L1 to another owner (like the original creators of that NFTs), which is not the way the L2Bridge work.

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 transfer ownership of a given collection created on L1

  • The admin will be able to transfer the ownership of that collection.

Impact

Inability to change ownership of Created NFT collections on L1

Tools Used

Manual Review

Recommendation

Implement a function to change the ownership of an 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..7d33554 100644
--- a/apps/blockchain/ethereum/src/Bridge.sol
+++ b/apps/blockchain/ethereum/src/Bridge.sol
@@ -374,4 +374,10 @@ contract Starklane is IStarklaneEvent, UUPSOwnableProxied, StarklaneState, Stark
emit L1L2CollectionMappingUpdated(collectionL1, snaddress.unwrap(collectionL2));
}
+ function collectionTransferOwnership(
+ ERC721Bridgeable collectionAddress,
+ address newAddress
+ ) external onlyOwner {
+ collectionAddress.transferOwnership(newAddress);
+ }
}

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.