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

+        // 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

    }

+    /// 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

        } 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

+    /// 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

    ///
    /// 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.

@dhaval-supraoracles , I see multiple modules where config_buffer:extract is being called. Can you please change them all to extract_v2 and compile/test ? supra_config.move, keyless_account.move and many others.

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

    ///
    /// 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

 /// 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

        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

    ///
    /// 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

    ///
    /// 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

        } 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

+    /// 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

        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

+        // 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

+        // 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.

sjoshisupra · 2026-02-13T07:24:11Z

    ///
    /// 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 , I see multiple modules where config_buffer:extract is being called. Can you please change them all to extract_v2 and compile/test ? supra_config.move, keyless_account.move and many others.

sjoshisupra · 2026-03-13T07:09:48Z

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


Isn't keeping it without public(friend) a vulnerability that was pointed out? I believe that since public interface can not be changed, we decided to keep the interface but use assert!(false, ENOT_SUPPORTED) for this and we should use a separate public(friend) that performs this buffer extraction, or use a public interface that takes a signer and check that it is framework signer. @supra-yoga

dhaval-supraoracles · 2026-03-13T07:50:34Z

+            let no_votes = proposal.no_votes;
+
+            if (yes_votes > no_votes &&
+                yes_votes + no_votes >= proposal.min_vote_threshold) {


here we want to consider yes+no as min vote threshold right?
not yes > min vote threshold.

dhaval-supraoracles · 2026-03-13T07:52:52Z

+    /// 10. `u64`                    - The time at which the task was registered (timestamp).
+    /// 11. `u8`                     - The state of the task (e.g., active, cancelled, completed).
+    /// 12. `u64`                    - The locked fee reserved for the next epoch execution.
    public fun deconstruct_task_metadata(


I think this will break the upgrade as public function signature is being changed.

Yes @dhaval Purohit, that change was intended. Just curious, can you kindly explain how updating the return types will break the upgrade?

I am assuming the users of these methods should mandatorily update their code to avoid this breakage.

Changing a public function's return type is considered a breaking change because:

External modules or scripts may already depend on the existing return type
The Move type system enforces strict type safety at the call site
Existing callers would break at runtime if the return type changed unexpectedly

sjoshisupra

What finally happened to extract and extract_v2? I do not see those in the diff. @supra-yoga

sjoshisupra · 2026-03-16T05:38:51Z

+    /// 9. `u64`                     - The time at which the task was registered (timestamp).
+    /// 10. `u8`                     - The state of the task (e.g., active, cancelled, completed).
+    /// 11. `u64`                    - The locked fee reserved for the next epoch execution.
+    public fun deconstruct_task_metadata(


deconstruct_task_metadata signature can not be changed during upgrade. Not sure if that is what is happening since I see above in the diff that decontruct_task_metadata is deleted and deconstruct_task_metadata_v2 is added. Can you confirm that original signature is being preserved? @supra-yoga

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 previously 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

supra-yoga added 2 commits March 12, 2026 11:05

Address review comments

5d9f0f3

Fix Move tests after patching

a45f30a

supra-yoga dismissed aregng’s stale review via a45f30a March 12, 2026 05:35

supra-yoga and others added 3 commits March 12, 2026 13:23

Merge branch 'dev' into issue-330

e543a8e

Merge branch 'dev' into issue-330

e1041ff

Fix multisig_voting.move audit issue

023d4f2

sjoshisupra requested changes Mar 13, 2026

View reviewed changes

anshuman-supraoracles previously approved these changes Mar 13, 2026

View reviewed changes

dhaval-supraoracles reviewed Mar 13, 2026

View reviewed changes

Add in automation_registry contract

390d7a8

supra-yoga dismissed anshuman-supraoracles’s stale review via 390d7a8 March 13, 2026 19:24

sjoshisupra reviewed Mar 16, 2026

View reviewed changes

dhaval-supraoracles approved these changes Mar 16, 2026

View reviewed changes

sjoshisupra self-requested a review March 16, 2026 06:00

sjoshisupra approved these changes Mar 16, 2026

View reviewed changes

supra-yoga merged commit f3040a4 into dev Mar 16, 2026
1 check passed

supra-yoga deleted the issue-330 branch March 16, 2026 06:00

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!

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!

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

supra-yoga Feb 12, 2026 •

edited

Loading

supra-yoga Feb 13, 2026 •

edited

Loading