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

No Storage Gap for Stark L2 Bridge

alqaqa

Vulnerability Details

In upgradable contracts, there should be a storage location for variables to get added freely without causing any storage collision. In Bridge.sol we can see that the contract inherits from a lot of contracts each of them has its own variables.

bridge.cairo#L62-L87

struct Storage {
...
// Bridge enabled flag
enabled: bool,
#[substorage(v0)]
❌️ ownable: OwnableComponent::Storage,
}

We are putting the ownable sub-storage after the main storage so if the Bridge storage took slot(0, 1, ..., 10). Ownable will take slots(11, 12). Any upgrade by adding a variable to the Bridge will result in Storage Collision.

The Bridge.cairo is intended to be an upgradable contract, where we can easily change its ClassHash from BridgeUpgradeImpl::upgrade.

bridge.cairo#L184-L198

impl BridgeUpgradeImpl of IUpgradeable<ContractState> {
fn upgrade(ref self: ContractState, class_hash: ClassHash) {
ensure_is_admin(@self);
​
❌️ match starknet::replace_class_syscall(class_hash) { ... };
}
}

So we will not be able to upgrade the contract and add new variables as Storaage Collision will occur.

The case is the same in erc721_bridgeable, where we are importing the sub storage in the beginning, and we do not have any gap.

erc721_bridgeable.cairo#L37-L47

struct Storage {
bridge: ContractAddress,
/// token_uris is required if we want uris not derivated from base_uri
token_uris: LegacyMap<u256, ByteArray>,
#[substorage(v0)]
erc721: ERC721Component::Storage,
#[substorage(v0)]
src5: SRC5Component::Storage,
#[substorage(v0)]
ownable: OwnableComponent::Storage,
}

Impact

Inability to upgrade these contracts if a new variable will be added.

Tools Used

Manual Review

Recommendations

  1. Move the Sub Storage from the Bottom to the Top, and make the main contract variables in the last

  2. Add a Gap to preserve Storage Slots for Variables that can be added in the future.

diff --git a/apps/blockchain/starknet/src/bridge.cairo b/apps/blockchain/starknet/src/bridge.cairo
index 23cbf8a..a12549c 100644
--- a/apps/blockchain/starknet/src/bridge.cairo
+++ b/apps/blockchain/starknet/src/bridge.cairo
@@ -60,6 +60,9 @@ mod bridge {
#[storage]
struct Storage {
+ #[substorage(v0)]
+ ownable: OwnableComponent::Storage,
+
// Bridge address on L1 (to allow it to consume messages).
bridge_l1_address: EthAddress,
// The class to deploy for ERC721 tokens.
@@ -81,9 +84,8 @@ mod bridge {
// Bridge enabled flag
enabled: bool,
-
- #[substorage(v0)]
- ownable: OwnableComponent::Storage,
+
+ __gap: [u128; 39],
}
#[constructor]

The same thing should be made for erc721_bridgeable.

Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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