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

`erc721_bridgeable` contracts can not update the address of the bridge as upgrading does not execute the constructor

fyamf

Summary

If for any reason the address of the bridge on L2 is changed (a new contract is deployed), the already deployed collections (i.e. erc721_bridgeable contracts) would not be able to update the address of the bridge. As a result, all the already deployed collections would keep the address of old bridge as a valid bridge, and unable to use the bridging mechanism by the new bridge.

Vulnerability Details

The reason is that when a erc721_bridgeable is upgraded, its constructor will not be executed again, only its class is replaced.

#[abi(embed_v0)]
impl ERC721BridgeableUpgradeImpl of IUpgradeable<ContractState> {
fn upgrade(ref self: ContractState, class_hash: ClassHash) {
self.ownable.assert_only_owner();
​
match starknet::replace_class_syscall(class_hash) {
Result::Ok(_) => (), // emit event
Result::Err(revert_reason) => panic(revert_reason),
};
}
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/token/erc721_bridgeable.cairo#L123

Since, the address of the bridge is initialized only during constructor (there is no admin function to update the address of the bridge), there is not any possibility to change the address of the bridge in a erc721_bridgeable contract.

#[constructor]
fn constructor(
ref self: ContractState,
name: ByteArray,
symbol: ByteArray,
base_uri: ByteArray,
bridge: ContractAddress,
collection_owner: ContractAddress,
) {
assert(!bridge.is_zero(), 'Invalid bridge address');
assert(!collection_owner.is_zero(), 'Bad collection owner address');
​
self.erc721.initializer(name, symbol, base_uri);
self.bridge.write(bridge);
self.ownable.initializer(collection_owner);
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/token/erc721_bridgeable.cairo#L61

Please note that such issue is not present for the Bridge contract. Because the admin can change all the critical state variables of Bridge which are initialized during constructor.

#[constructor]
fn constructor(
ref self: ContractState,
bridge_admin: ContractAddress,
bridge_l1_address: EthAddress,
erc721_bridgeable_class: ClassHash,
) {
self.ownable.initializer(bridge_admin);
// TODO: add validation of inputs.
self.bridge_l1_address.write(bridge_l1_address);
self.erc721_bridgeable_class.write(erc721_bridgeable_class);
self.white_list_enabled.write(false);
self.enabled.write(false); // disabled by default
}

https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L90
https://github.com/Cyfrin/2024-07-ark-project/blob/main/apps/blockchain/starknet/src/bridge.cairo#L200-L365

Please note that one possible solution is that Bridge upgrades the class of every already deployed erc721_bridgeable contract to include a function (like set_bridge_l2_addr) to be able to change the address of bridge. Then, the class of Bridge be upgraded to include a function (like collection_bridge_upgrade) to be able to call any erc721_bridgeable::set_bridge_l2_addr.
But, problem with such solution is that upgrading the class of every already deployed erc721_bridgeable is not straightforward (if there are many deployed ones).

Impact

  • If the address of L2 bridge is changed, all the already deployed collections on L2 would not be possible to interact with this new bridge.

Tools Used

Recommendations

The following function should be added to erc721_bridgeable contract:

+ fn set_bridge_l2_addr(ref self: ContractState, bridge: ContractAddress) {
+ self.ownable.assert_only_owner();
+ self.bridge.write(bridge);
+ }

Moreover, the following function should be added to bridge contract:

+ fn collection_bridge_upgrade(ref self: ContractState, collection: ContractAddress, bridge: ContractAddress) {
+ ensure_is_admin(@self);
+ IUpgradeableDispatcher { contract_address: collection }
+ .set_bridge_l2_addr(bridge);
+ }
Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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