ArkProject: NFT Bridge

Github Links
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L184-L198
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/token/erc721_bridgeable.cairo#L123-L132

Severity

likelihood: low
impact: very high

Summary

As opposed to solidity UUPSUpgradeability pattern, the starknet bridge.cairo contract can be upgraded to a class hash of a contract that is non upgradeable. if this happens to be a bad/wrong class hash, all tokens held by the bridge would be lost for good. Similarly if the erc721_bridgeable.cairois upgraded to a wrong unupgradeable class hash, the contract might become bricked and all tokens lost.

Vulnerability Details

Solidity uups upgradeability pattern ensures that upgrades can only be made to new uups implementations
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/1edc2ae004974ebf053f4eba26b45469937b9381/contracts/proxy/utils/UUPSUpgradeable.sol#L138

basically ensuring you dont mistakingly upgrade to a non upgradeable contract.

It is much better to revoke ownership of a contract than to upgrade to a non upgradeable version.

Something similar should be done with the cairo smart contracts. Currently there are no similar checks in the affected cairo contracts

Impact

if the l2 bridge.cairo contract is mistakingly upgraded to the wrong class hash, it would lead to the loss of all the nfts held by the contract. Similarly, if the erc721_bridgeable.cairo contracts are upgraded to the wrong class hash, the nft contract affected would be lost.

if this mistake is made, the damage will be irreparable. and this mistake has been made before.

Recommendation

We can do something like this for both bridge.cairo and erc721_bridgeable.cairo