Skip to content

fix: token audit#750

Open
charmful0x wants to merge 23 commits intoimpr/secure-actionsfrom
audit/token
Open

fix: token audit#750
charmful0x wants to merge 23 commits intoimpr/secure-actionsfrom
audit/token

Conversation

@charmful0x
Copy link

@charmful0x charmful0x commented Mar 12, 2026

findings

1- fix: transfer/3 was overly permissive on the recipient value (via validate_address/1) where the only check was is_binary and size > 0 (no upper size limit). and the edge case was, when recipient equals one of the dev_trie reserved keys (<<"device">> etc), the balances state would corrupt after a transfer targeting a reserved key.

2- fix: send_error/4 API misalignment and Reason types under coverage. send error usage was inconsistent across compute/3 and transfer/3 -- and it only handled Reason type binary where it could atom (not_found) or a tuple from handle_action/4

3- fix: balance/3 return shape was mixed between {ok, {ok, IntegerBalance}} when the requested Account has a balance (via raw return hb_ao:resolve_many()), and {ok, {error, not_found}} when its a miss.

4- chore: unsupported Actions did silently no-op instead of explicitly logging what unsupported actions has been called.

5- chore: enforce_set_authority/3 had a loose match order where, if both request from and base set-authority resolved to not_found, the first case branch matched and incorrectly returned true

fixes

1- address must be a binary with 0 < size <= 128 bytes (which fit AR/AO, EVM and SOL addresses lengths). and the address value must not be one of the dev_trie reserved edge keys.

reserved_keys() ->
    [   <<"node-value">>,
        <<"device">>,
        <<"commitments">>,
        <<"priv">>,
        <<"hashpath">>
    ].

2- send_error/4 now returns a consistent {ok, Base} style result and its usage in transfer/3 was updated to match that API

3- aligned the balance/3 return shape for consistency, as follow:

    case Balance of
        {ok, Balance} -> {ok, Balance};
        {error, not_found} -> {ok, 0};
        {error, Reason} -> {error, Reason}
    end.

4- on action_as_mint_device/4 false path, it propagates the error of unsupported action via send_error/4
5- strict case branch logic now check missing normalized from, missing set-authority, exact match, and mismatch explicitly, as follow:

    case {Setter, SetAuthority} of
       {not_found, _} -> 
           {error, <<"No normalized `from` key found in request.">>};
       {_, not_found} -> 
           {error, <<"No set-authority found in `Base' state.">>};
       {S, S} -> 
           true;
       _ -> 
           {error, <<"Caller is not the `set-authority'.">>}
   end.

true ?= (is_integer(SenderBalance) and is_integer(RecipientBalance)
and (SenderBalance >= 0) and (RecipientBalance >= 0))
orelse {error, <<"Invalid balance values.">>},
true ?= (is_integer(Quantity) and (Quantity >= 0))
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we allow 0 transfers? or atleast in the case of $AO? this could be a spam entrypoint..?

ETH and ERC-20 spec allows 0 transfers due to the existence of the associated gas cost

@charmful0x
Copy link
Author

===> Performing EUnit tests...
======================== EUnit ========================
module 'dev_token_test_vectors'
  dev_token_test_vectors: transfer_basic_test...[3.324 s] ok
  dev_token_test_vectors: simple_process_test...[4.612 s] ok
  dev_token_test_vectors: simple_pot_process_test...[3.784 s] ok
  dev_token_test_vectors: benchmark_process_transfers...
        Process transfers 100 transfers in 2,453ms (41 transfers/s)... [15.607 s] ok
  [done in 27.339 s]
=======================================================
  All 4 tests passed.

@charmful0x charmful0x changed the title wip: token audit fix: token audit Mar 17, 2026
@charmful0x
Copy link
Author

======================== EUnit ========================
module 'dev_token_test_vectors'
  dev_token_test_vectors: transfer_basic_test...[3.051 s] ok
  dev_token_test_vectors: simple_process_test...[3.682 s] ok
  dev_token_test_vectors: reserved_recipient_transfer_rejected_test...[2.770 s] ok
  dev_token_test_vectors: simple_pot_process_test...[5.072 s] ok
  dev_token_test_vectors: benchmark_process_transfers...
        Process transfers 100 transfers in 3,168ms (32 transfers/s)... [22.212 s] ok
  [done in 36.802 s]
=======================================================
  All 5 tests passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant