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

Bridges cannot be upgraded

SBSecurity

Summary

Due to Bridge and bridge.cairo contracts both being used as Escrow as well, upgrades won’t be possible since all the NFTs bridged will be held in them.

Vulnerability Details

Looking at the current implementation of the Bridge and bridge.cairo contracts it’s clear that the Ark team intends to perform regular upgrades. In fact, probably the biggest upgrade will be when ERC1155 support is added.

Bridge.sol (inherited from UUPSProxied)

/**
* @dev Upgrade the implementation of the proxy to `newImplementation`.
*
* Calls {_authorizeUpgrade}.
*
* Emits an {Upgraded} event.
*
* @custom:oz-upgrades-unsafe-allow-reachable delegatecall
*/
function upgradeTo(address newImplementation) public virtual onlyProxy {
_authorizeUpgrade(newImplementation);
_upgradeToAndCallUUPS(newImplementation, new bytes(0), false);
}

bridge.cairo

#[abi(embed_v0)]
impl BridgeUpgradeImpl of IUpgradeable<ContractState> {
fn upgrade(ref self: ContractState, class_hash: ClassHash) {
ensure_is_admin(@self);
​
match starknet::replace_class_syscall(class_hash) {
Result::Ok(_) => {
self.emit(ReplacedClassHash {
contract: starknet::get_contract_address(),
class: class_hash,
})
},
Result::Err(revert_reason) => panic(revert_reason),
};
}
}

The most important note regarding this issue is the fact that the admin does not have any control over the escrowed tokens, he can’t retrieve them or force withdraw. Users are also not obligated to have a maximum period after which they should bridge back their NFTs to the originator chain, quite the opposite, tokens can stay escrowed forever if they bring benefit for their owner on the bridged chain.

The issue itself is the fact that tokens, when escrowed are kept in the bridges directly, not in another contract, which will lead to the following 2 scenarios:

  • Admins can’t upgrade because if the implementation is changed all the escrowed tokens will be lost since their owner will be the old implementation contract.

  • If an emergency upgrade is needed because of a bug in the Bridge contract or Starknet changes their architecture and this makes all the escrowed tokens inaccessible.

Not matter the reason for the upgrade, issues will occur and will harm both the system and users.

Impact

  • Inability from admins to upgrade the bridge contracts

  • Loss of funds/assets from users who have bridged their tokens

Tools Used

Manual Review

Recommendations

You can separate the Bridge and Escrow into 2 different contracts.

Here is how MakerDAO, achieves this for their Starknet contracts: https://github.com/makerdao/starknet-dai-bridge/blob/380a6ed2b78644b05c8b9fd0e0f969da1a5cadf9/contracts/l1/L1DAIBridge.sol

The L1DAIBridge contract implements all logic to deposit and withdraw DAI to and from L2. The deposited DAI are held by the L1Escrow contract. This separation of the funds from the logic makes the system upgradable: the L1DAIBridge logic contract can be replaced independently of the Escrow holding the DAI.
Similarly, the l2_dai_bridge contract implements the logic for handling deposits and withdrawals. Note that on L2 the bridge can mint and burn DAI and no escrow is needed on L2. Also, the bridge on L2 can be upgraded through a governance spell through the l2_governance_relay contract that could remove the bridge's minting and burning rights and assign them to a new bridge.

Updates

Lead Judging Commences

n0kto Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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