NFTBridge
60,000 USDC
View results
Submission Details
Severity: low
Invalid

Important state variables in Bridge.sol will be unintentionally overwritten due to absence of storage gaps

Summary

The Ark contract was written with UUPS library to facilitate universal upgrades. Notably, upgradeability in smart contracts treats almost everything as storage slots, including state variables. Meanwhile, OpenZeppelin recommends the intentional creation of storage gaps so that new implementation contracts will work as expected and every variable will be in its expected slot. But the problem here is that no gap storage was left in Ark!

Vulnerability Details

Specifically, the vulnerability is here:

contract Starklane is IStarklaneEvent, UUPSOwnableProxied, StarklaneState, StarklaneEscrow, StarklaneMessaging, CollectionManager {
// Mapping (collectionAddress => bool)
mapping(address => bool) _whiteList;
address[] _collections;
bool _enabled;
bool _whiteListEnabled;

As the parent contract, no storage gap was provided there.

Impact

The impact of this vulnerability can be serious as new state variables in the upgraded contract can overwrite state variables in child contracts. Here is a Judicial Precedent.

Proof of Concept

Assume a StarklaneV2 contract is deployed as an upgrade to Starklane, and a new boolean variable blacklistis added before mapping(address => bool) _whiteListin StarklaneV2.

The new blacklistwill overwrite the _whitelistmap due to storage slot push-down, thereby disrupting what the _whitelistmap does in the contract.

This is only one of the many things that can go wrong!

Tools Used

Manual review.

Recommendations

Follow the security and development best practices OpenZeppelin, the creator of the library, provided here

Practically, here is how to fix this bug:

contract Starklane is IStarklaneEvent, UUPSOwnableProxied, StarklaneState, StarklaneEscrow, StarklaneMessaging, CollectionManager {
// Mapping (collectionAddress => bool)
mapping(address => bool) _whiteList;
address[] _collections;
bool _enabled;
bool _whiteListEnabled;
+ uint256[60] __gap; // this reserves 60 storage slots to cater for new implementation contracts
Updates

Lead Judging Commences

n0kto Lead Judge 9 months ago
Submission Judgement Published
Invalidated
Reason: Known issue
Assigned finding tags:

invalid-upgradeable-storage-gap-known-issue

Known issue: Lightchaser

Support

FAQs

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