Fixes for issues reported in pentesting audit by supra-yoga · Pull Request #338 · Entropy-Foundation/aptos-core

supra-yoga · 2026-02-12T07:34:43Z

Resolves all issues in #330

anshuman-supraoracles · 2026-02-12T08:10:06Z

aptos-move/framework/supra-framework/sources/fungible_asset.move

+        // Validate all provided values before mutating any metadata fields.
+        if (option::is_some(&name)){
+            let new_name = option::borrow(&name);
+            assert!(string::length(new_name) <= MAX_NAME_LENGTH, error::out_of_range(ENAME_TOO_LONG));
+        };
+        if (option::is_some(&symbol)){
+            let new_symbol = option::borrow(&symbol);
+            assert!(string::length(new_symbol) <= MAX_SYMBOL_LENGTH, error::out_of_range(ESYMBOL_TOO_LONG));
+        };
+        if (option::is_some(&decimals)){
+            let new_decimals = option::borrow(&decimals);
+            assert!(*new_decimals <= MAX_DECIMALS, error::out_of_range(EDECIMALS_TOO_LARGE));
+        };
+        if (option::is_some(&icon_uri)){
+            let new_icon_uri = option::borrow(&icon_uri);
+            assert!(string::length(new_icon_uri) <= MAX_URI_LENGTH, error::out_of_range(EURI_TOO_LONG));
+        };
+        if (option::is_some(&project_uri)){
+            let new_project_uri = option::borrow(&project_uri);
+            assert!(string::length(new_project_uri) <= MAX_URI_LENGTH, error::out_of_range(EURI_TOO_LONG));
+        };
+


Rather than 2 conditional statement for a same cause

Can we just update the values after the assert

For example

if (option::is_some(&name)){ let new_name = option::extract(&mut name); assert!(string::length(new_name) <= MAX_NAME_LENGTH, error::out_of_range(ENAME_TOO_LONG)); mutable_metadata.name = new_name; };

Can we use destroy_some instead of extract Dr @sjoshisupra ?

In that case, if the 2nd or 3rd assertion fails, the 1st metadata would've been set incorrectly already. Therefore, checking for all the metadata limits before assigning them might be better, no?

I am not sure how the Move compiler will process this flow.

@supra-yoga if one of the asserts gets failed then the txn will get failed so there wont be any partial state update

@anshuman-supraoracles , @supra-yoga ,

Yes we can use destroy_some instead of extract but we need to know that the caller has no use of the same after the call. extract will make it option::none, whereas destroy_some would destroy the option altogether.

I agree, no need to perform option::is_some check twice for every field. If anything fails, the whole tx reverts anyway.

Understood. Will update it as per the suggestion and also use option::destroy_some. Thanks @anshuman-supraoracles.

aregng

Approved automation_registry changes.
Left couple of suggestions to be fixed based on applicability.

aregng · 2026-02-12T08:16:45Z

aptos-move/framework/supra-framework/sources/committee_map.move

    }

+    /// Ensures removing exactly one member keeps the committee type invariants valid.
+    fun validate_committee_type_after_member_removal(committee: &CommitteeInfo) {


Suggestion for the function name: assert_removal_allowed

aregng · 2026-02-12T08:18:12Z

aptos-move/framework/supra-framework/sources/committee_map.move

        } else if (committee_type == CLAN) {
-            // 2f+1, number of nodes in a clan committee should be odd and greater than 3
+            // 2f+1, number of nodes in a clan committee should be odd and greater than or equal to 3
            assert!(num_of_nodes >= 3 && num_of_nodes % 2 == 1, INVALID_NODE_NUMBERS);


INVALID_NODE_NUMBERS sound to generic, maybe error per committee type will be more descriptive. e.g. INVALID_CLAN_NODE_NUMBERS, INVALID_FAMILIY_NODE_NUMBERS

ECLAN_TOO_SMALL_OR_EVEN_NODES_IN_CLAN

aregng · 2026-02-12T08:19:03Z

aptos-move/framework/supra-framework/sources/committee_map.move

+    /// Ensures removing exactly one member keeps the committee type invariants valid.
+    fun validate_committee_type_after_member_removal(committee: &CommitteeInfo) {
+        let current_num_of_nodes = simple_map::length(&committee.map);
+        assert!(current_num_of_nodes > 0, INVALID_NODE_NUMBERS);


and here INVALID_NODE_NUMBERS ZERO_COMMITTE_NODE_NUMBER

EZERO_NUM_NODE let us use E prefix for error codes.

dhaval-supraoracles · 2026-02-12T08:29:58Z

aptos-move/framework/supra-framework/sources/configs/config_buffer.move

    ///
    /// Typically used in `X::on_new_epoch()` where X is an on-chaon config.
-    public fun extract<T: store>(): T acquires PendingConfigs {
+    public(friend) fun extract<T: store>(): T acquires PendingConfigs {


With this we can't upgrade I think.
we need to have another interface for the same.

which is included in fwk upgrade PR LINK

@dhaval-supraoracles , has the necessary changes been done to the calling code? Are they now calling extract_v2? I do not see that in the PR link you mentioned above.

yes all relevant module also uses extract_v2 on the link that I share.

isaacdoidge · 2026-02-12T09:05:59Z

aptos-move/framework/supra-framework/sources/configs/config_buffer.move

    ///
    /// Typically used in `X::on_new_epoch()` where X is an on-chaon config.
-    public fun extract<T: store>(): T acquires PendingConfigs {
+    public(friend) fun extract<T: store>(): T acquires PendingConfigs {


All modules that call this function need to be marked as friends of this one. Have they already been marked as such? Also, I'm not sure if this might prevent us from using this function in scripts.

I think a simpler change would be to add system_addresses::assert_supra_framework(framework); as the first line of the function instead.

Ah, since the method does not take a signer it is not possible to do assert_supra_framework, we have to make changes to all the callers.

@dhaval-supraoracles Can we make this change in the upcoming FWK upgrade?

It's already updated on FWK upgrade changes. @supra-yoga

isaacdoidge · 2026-02-12T09:16:58Z

aptos-move/framework/src/natives/aggregator_natives/aggregator_v2.rs

 /// The maximum length of the input string for derived string snapshot.
 /// If we want to increase this, we need to modify BITS_FOR_SIZE in types/src/delayed_fields.rs.
-pub const DERIVED_STRING_INPUT_MAX_LENGTH: usize = 1024;
+pub const DERIVED_STRING_INPUT_MAX_LENGTH: usize = 256;


This is a breaking change to the execution semantics and needs to be feature gated. It would be simpler to update the docs to reflect the actual 1024 byte limit, if that is the only place that it is referenced. This will also avoid breaking any objects that have already been created in mainnet with a length between 256 and 1024 bytes. We need to check if any other code expects 256 bytes before doing this.

isaacdoidge · 2026-02-12T09:22:43Z

aptos-move/framework/supra-framework/sources/committee_map.move

        let committee_store = borrow_global_mut<CommitteeInfoStore>(com_store_addr);
+        let event_handler = borrow_global_mut<SupraCommitteeEventHandler>(get_committeeInfo_address(owner_signer));
+
+        // If the node is already associated with another committee, remove the stale entry first.


I'm not sure how we're using this module, but I know that when we eventually add support for Clans and Families (sub-committees) to the L1 that a node will be able to participate in multiple committees simultaneously. If we intend to use this module for that use case then node_to_committee_map should be updated from SimpleMap<address, u64> to SimpleMap<address, vector<u64>> instead. Perhaps Dr @sjoshisupra knows more about the intended purpose of this module.

For now, this is used for DORA nodes for discovery @dhaval-supraoracles , but yes, we can modify it to make it more general.

sjoshisupra

@aregng are other issues from the pentest report wrt, gas tracking and estimate handled already?

sjoshisupra · 2026-02-12T09:58:26Z

aptos-move/framework/supra-framework/sources/configs/config_buffer.move

    ///
    /// Typically used in `X::on_new_epoch()` where X is an on-chaon config.
-    public fun extract<T: store>(): T acquires PendingConfigs {
+    public(friend) fun extract<T: store>(): T acquires PendingConfigs {


@dhaval-supraoracles , has the necessary changes been done to the calling code? Are they now calling extract_v2? I do not see that in the PR link you mentioned above.

sjoshisupra · 2026-02-12T09:58:54Z

aptos-move/framework/supra-framework/sources/configs/config_buffer.move

    ///
    /// Typically used in `X::on_new_epoch()` where X is an on-chaon config.
-    public fun extract<T: store>(): T acquires PendingConfigs {
+    public(friend) fun extract<T: store>(): T acquires PendingConfigs {


sjoshisupra · 2026-02-12T10:00:27Z

aptos-move/framework/supra-framework/sources/committee_map.move

        } else if (committee_type == CLAN) {
-            // 2f+1, number of nodes in a clan committee should be odd and greater than 3
+            // 2f+1, number of nodes in a clan committee should be odd and greater than or equal to 3
            assert!(num_of_nodes >= 3 && num_of_nodes % 2 == 1, INVALID_NODE_NUMBERS);


ECLAN_TOO_SMALL_OR_EVEN_NODES_IN_CLAN

sjoshisupra · 2026-02-12T10:01:46Z

aptos-move/framework/supra-framework/sources/committee_map.move

+    /// Ensures removing exactly one member keeps the committee type invariants valid.
+    fun validate_committee_type_after_member_removal(committee: &CommitteeInfo) {
+        let current_num_of_nodes = simple_map::length(&committee.map);
+        assert!(current_num_of_nodes > 0, INVALID_NODE_NUMBERS);


EZERO_NUM_NODE let us use E prefix for error codes.

sjoshisupra · 2026-02-12T10:03:05Z

aptos-move/framework/supra-framework/sources/committee_map.move

        let committee_store = borrow_global_mut<CommitteeInfoStore>(com_store_addr);
+        let event_handler = borrow_global_mut<SupraCommitteeEventHandler>(get_committeeInfo_address(owner_signer));
+
+        // If the node is already associated with another committee, remove the stale entry first.


For now, this is used for DORA nodes for discovery @dhaval-supraoracles , but yes, we can modify it to make it more general.

sjoshisupra · 2026-02-12T10:10:55Z

aptos-move/framework/supra-framework/sources/fungible_asset.move

+        // Validate all provided values before mutating any metadata fields.
+        if (option::is_some(&name)){
+            let new_name = option::borrow(&name);
+            assert!(string::length(new_name) <= MAX_NAME_LENGTH, error::out_of_range(ENAME_TOO_LONG));
+        };
+        if (option::is_some(&symbol)){
+            let new_symbol = option::borrow(&symbol);
+            assert!(string::length(new_symbol) <= MAX_SYMBOL_LENGTH, error::out_of_range(ESYMBOL_TOO_LONG));
+        };
+        if (option::is_some(&decimals)){
+            let new_decimals = option::borrow(&decimals);
+            assert!(*new_decimals <= MAX_DECIMALS, error::out_of_range(EDECIMALS_TOO_LARGE));
+        };
+        if (option::is_some(&icon_uri)){
+            let new_icon_uri = option::borrow(&icon_uri);
+            assert!(string::length(new_icon_uri) <= MAX_URI_LENGTH, error::out_of_range(EURI_TOO_LONG));
+        };
+        if (option::is_some(&project_uri)){
+            let new_project_uri = option::borrow(&project_uri);
+            assert!(string::length(new_project_uri) <= MAX_URI_LENGTH, error::out_of_range(EURI_TOO_LONG));
+        };
+


@anshuman-supraoracles , @supra-yoga ,

Yes we can use destroy_some instead of extract but we need to know that the caller has no use of the same after the call. extract will make it option::none, whereas destroy_some would destroy the option altogether.

sjoshisupra · 2026-02-12T10:12:10Z

aptos-move/framework/supra-framework/sources/fungible_asset.move

+        // Validate all provided values before mutating any metadata fields.
+        if (option::is_some(&name)){
+            let new_name = option::borrow(&name);
+            assert!(string::length(new_name) <= MAX_NAME_LENGTH, error::out_of_range(ENAME_TOO_LONG));
+        };
+        if (option::is_some(&symbol)){
+            let new_symbol = option::borrow(&symbol);
+            assert!(string::length(new_symbol) <= MAX_SYMBOL_LENGTH, error::out_of_range(ESYMBOL_TOO_LONG));
+        };
+        if (option::is_some(&decimals)){
+            let new_decimals = option::borrow(&decimals);
+            assert!(*new_decimals <= MAX_DECIMALS, error::out_of_range(EDECIMALS_TOO_LARGE));
+        };
+        if (option::is_some(&icon_uri)){
+            let new_icon_uri = option::borrow(&icon_uri);
+            assert!(string::length(new_icon_uri) <= MAX_URI_LENGTH, error::out_of_range(EURI_TOO_LONG));
+        };
+        if (option::is_some(&project_uri)){
+            let new_project_uri = option::borrow(&project_uri);
+            assert!(string::length(new_project_uri) <= MAX_URI_LENGTH, error::out_of_range(EURI_TOO_LONG));
+        };
+


I agree, no need to perform option::is_some check twice for every field. If anything fails, the whole tx reverts anyway.

Fixes for issues reported in pentesting audit

cf6ba8c

supra-yoga requested review from anshuman-supraoracles, isaacdoidge, nizam-supraoracles and sjoshisupra February 12, 2026 07:34

supra-yoga self-assigned this Feb 12, 2026

anshuman-supraoracles requested changes Feb 12, 2026

View reviewed changes

aregng approved these changes Feb 12, 2026

View reviewed changes

dhaval-supraoracles reviewed Feb 12, 2026

View reviewed changes

isaacdoidge reviewed Feb 12, 2026

View reviewed changes

sjoshisupra requested changes Feb 12, 2026

View reviewed changes

Conversation

supra-yoga commented Feb 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

supra-yoga Feb 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

supra-yoga Feb 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aregng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjoshisupra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

supra-yoga Feb 12, 2026 •

edited

Loading

supra-yoga Feb 13, 2026 •

edited

Loading