Attackathon _ Fuel Network 32612 - [Smart Contract - Low] Lack of slot hashing at adminsw can cause

Submitted on Thu Jun 27 2024 11:17:22 GMT-0400 (Atlantic Standard Time) by @jecikpo for Attackathon | Fuel Network

Report ID: #32612

Report type: Smart Contract

Report severity: Low

Target: https://github.com/FuelLabs/sway-libs/tree/0f47d33d6e5da25f782fc117d4be15b7b12d291b

Impacts:

• Unauthorized minting of NFTs

• Access Control bypass

• Permanent freezing of NFTs

• Permanent freezing of funds

Description

Brief/Intro

The StorageKey is used at sway-libs/libs/src/admin.sw and it allows to place various values under different slots in storage. Usage of Identity as key in StorageKey and lack of any hashing of the key can cause predictable storage slots and hence storage slot collisions in case Identity is re-used by 3rd party libs to store anything, which can be easily abused based on specific dapp implementations.

Vulnerability Details

StorageKey library allows to store data on the contract storage. StorageKey object contains three members:

slot: [b256] - The assigned location in storage for the new StorageKey. offset: [u64] - The assigned offset based on the data structure T for the new StorageKey. field_id: [b256] - A unique identifier for the new StorageKey. The problem is that in the new() function the slot is assigned and it is not changed in any way:

pub fn new(slot: b256, offset: u64, field_id: b256) -> Self {
        Self {
            slot,
            offset,
            field_id,
        }
    }

Also the write() function takes the slot directly:

    pub fn write(self, value: T) {
        write(self.slot(), self.offset(), value);
    }

The issue here is that if the slot is taken exactly as is from the Identity. If there is any other 3rd party library which also puts information on the storage using some Identity as key, it can easily be used to overwrite the existing admin account information and hence breaking the access control mechanisms.

Here we can see how a new admin account is added in the add_admin():

    let admin_key = StorageKey::<Identity>::new(new_admin.bits(), 0, new_admin.bits());
    admin_key.write(new_admin);

The expectation here would be that the slot and field_id would be hashed together to form the actual storage slot location. This way, if the field_id is hardcoded, the impact of storage writing/reading is contained within the contract in a well defined scope. While StorageKey is not using field_id by design, it would be recommended to make use of StorageMap to hold the admin accounts information in the storage.

Switching from StorageKey to StorageMap makes the implementation in line with sway-libs/libs/src/ownership.sw which uses a hash digest of sha256("owner") as the holding slot of the owner.

The decision to use StorageKey instead of StorageMap was based on lower gas consumption of StorageKey vs StorageMap. This however compromised security based on the above description. Gas usage normally should not be considered for access control guarded function because they are mostly rarely called and only by the protocol team's approved accounts.

Impact Details

The impact of this is highly dependent on the actual dapp implementation, however as this is a low level function it could easily be used to overwrite access to minting Coins or NFTs issuance functions. Usage of admin typically controls also the upgrade process of a protocol and so could lead to a destruction of a protocol if used by a malicious user (hence the permanent freeze of assets impact listed)

We can imagine that there is another 3rd party library which uses Identity to store user data, a user could put there data that would be his own identity. only_admin() could then be reading that data and assume that the normal user is a real admin.

I chose the impact as critical, because the security of admin.sw implementation in the current form is depending on the 3rd party libraries implementations (so that they won't use StorageKey in that certain way) in the entire future ecosystem. and admin.sw could become a critical component of almost every dapp built on Fuel.

Regarding the likeliness of the abuse, in order to keep the current implementation secure, no other external library or code would use Identity as slot argument of the StorageKey, which is a requirement that would be hard to enforce.

References

StorageKey::new() function: https://github.com/FuelLabs/sway/blob/7b56ec734d4a4fda550313d448f7f20dba818b59/sway-lib-core/src/storage.sw#L45C5-L51C6

StorageKey::write() function: https://github.com/FuelLabs/sway/blob/7b56ec734d4a4fda550313d448f7f20dba818b59/sway-lib-std/src/storage/storage_key.sw#L56C5-L58C6

admin.sw implementing add_admin() function: https://github.com/FuelLabs/sway-libs/blob/0f47d33d6e5da25f782fc117d4be15b7b12d291b/libs/src/admin.sw#L37C8-L37C17

Proof of concept

Proof of Concept

The following gist contains the PoC: https://gist.github.com/jecikpo/9d68846ec310727caca86ce187444523