[framework] Incorrect Immutability Validation Prevents Metadata Updates in CoinManager

[miker83z]

FROM THE AUDIT:

Summary

The functions update_name, update_symbol, update_description, and update_icon_url in the CoinManager incorrectly enforce an immutable metadata check, preventing updates to mutable metadata.

Vulnerability Detail

The metadata_is_immutable function in the CoinManager determines whether metadata is immutable based on two conditions:

public fun metadata_is_immutable<T>(manager: &CoinManager<T>): bool {
    manager.metadata_immutable || option::is_some(&manager.immutable_metadata)
}

The function incorrectly evaluates to true if either metadata_immutable is set to true or immutable_metadata contains a value (Some). This logic is used in the update_name, update_symbol, update_description, and update_icon_url functions as follows:

public fun update_name<T>(
    _: &CoinManagerMetadataCap<T>,
    manager: &mut CoinManager<T>,
    name: string::String
) {
    assert!(manager.metadata_is_immutable(), ENoMutableMetadata); // Incorrect check
    coin::update_name(&manager.treasury_cap, option::borrow_mut(&mut manager.metadata), name)
}

The assertion expects metadata_is_immutable to return false for updates to proceed. However, due to the flawed logic, the function blocks updates even when metadata is mutable. The same logic error applies to the other mentioned functions.

POC

    #[test]
    #[expected_failure(abort_code = coin_manager::ENoMutableMetadata)]
    fun test_coin_manager_immutable_bug() {
        let sender = @0xA;
        let mut scenario = test_scenario::begin(sender);
        let witness = COIN_MANAGER_TESTS{};

        // Create a `Coin`.
        let (cap, meta) = coin::create_currency(
            witness,
            0,
            b"TEST",
            b"TEST",
            b"TEST",
            option::none(),
            scenario.ctx(),
        );

        // Create CoinManager
        let (cmcap, metacap, mut wrapper) = coin_manager::new(cap, meta, scenario.ctx());

        //@audit Try Updating Name - Reverts
        metacap.update_name(&mut wrapper, utf8(b"Buggu"));

        // Destorying / Transferring Stuff
        transfer::public_transfer(cmcap, scenario.ctx().sender());
        metacap.renounce_metadata_ownership(&mut wrapper);
        transfer::public_share_object(wrapper);

        scenario.end();
    }

Impact

• Prevents Legitimate Updates: Metadata updates fail unnecessarily even when updates should be allowed, disrupting intended functionality.

Code Snippet

    // === Update coin metadata ===

    /// Update the `name` of the coin in the `CoinMetadata`.
    public fun update_name<T>(
        _: &CoinManagerMetadataCap<T>,
        manager: &mut CoinManager<T>,
        name: string::String
    ) {
@>>     assert!(manager.metadata_is_immutable(), ENoMutableMetadata);
        coin::update_name(&manager.treasury_cap, option::borrow_mut(&mut manager.metadata), name)
    }

    /// Update the `symbol` of the coin in the `CoinMetadata`.
    public fun update_symbol<T>(
        _: &CoinManagerMetadataCap<T>,
        manager: &mut CoinManager<T>,
        symbol: ascii::String
    ) {
@>>     assert!(manager.metadata_is_immutable(), ENoMutableMetadata);
        coin::update_symbol(&manager.treasury_cap, option::borrow_mut(&mut manager.metadata), symbol)
    }

    /// Update the `description` of the coin in the `CoinMetadata`.
    public fun update_description<T>(
        _: &CoinManagerMetadataCap<T>,
        manager: &mut CoinManager<T>,
        description: string::String
    ) {
@>>     assert!(manager.metadata_is_immutable(), ENoMutableMetadata);
        coin::update_description(&manager.treasury_cap, option::borrow_mut(&mut manager.metadata), description)
    }

    /// Update the `url` of the coin in the `CoinMetadata`
    public fun update_icon_url<T>(
        _: &CoinManagerMetadataCap<T>,
        manager: &mut CoinManager<T>,
        url: ascii::String
    ) {
@>>     assert!(manager.metadata_is_immutable(), ENoMutableMetadata);
        coin::update_icon_url(&manager.treasury_cap, option::borrow_mut(&mut manager.metadata), url)
    }

Tool used

Manual Review

Recommendation

    // === Update coin metadata ===

    /// Update the `name` of the coin in the `CoinMetadata`.
    public fun update_name<T>(
        _: &CoinManagerMetadataCap<T>,
        manager: &mut CoinManager<T>,
        name: string::String
    ) {
-       assert!(manager.metadata_is_immutable(), ENoMutableMetadata);
        coin::update_name(&manager.treasury_cap, option::borrow_mut(&mut manager.metadata), name)
    }

    /// Update the `symbol` of the coin in the `CoinMetadata`.
    public fun update_symbol<T>(
        _: &CoinManagerMetadataCap<T>,
        manager: &mut CoinManager<T>,
        symbol: ascii::String
    ) {
-       assert!(manager.metadata_is_immutable(), ENoMutableMetadata);
        coin::update_symbol(&manager.treasury_cap, option::borrow_mut(&mut manager.metadata), symbol)
    }

    /// Update the `description` of the coin in the `CoinMetadata`.
    public fun update_description<T>(
        _: &CoinManagerMetadataCap<T>,
        manager: &mut CoinManager<T>,
        description: string::String
    ) {
-       assert!(manager.metadata_is_immutable(), ENoMutableMetadata);
        coin::update_description(&manager.treasury_cap, option::borrow_mut(&mut manager.metadata), description)
    }

    /// Update the `url` of the coin in the `CoinMetadata`
    public fun update_icon_url<T>(
        _: &CoinManagerMetadataCap<T>,
        manager: &mut CoinManager<T>,
        url: ascii::String
    ) {
-      assert!(manager.metadata_is_immutable(), ENoMutableMetadata);
        coin::update_icon_url(&manager.treasury_cap, option::borrow_mut(&mut manager.metadata), url)
    }