diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 29b505f7..77436d7d 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -31,10 +31,10 @@ }, { "name": "building-secure-contracts", - "version": "1.0.1", + "version": "1.1.0", "description": "Comprehensive smart contract security toolkit based on Trail of Bits' Building Secure Contracts framework. Includes vulnerability scanners for 6 blockchains and 5 development guideline assistants.", "author": { - "name": "Omar Inuwa" + "name": "Omar Inuwa && Paweł Płatek" }, "source": "./plugins/building-secure-contracts" }, diff --git a/plugins/building-secure-contracts/.claude-plugin/plugin.json b/plugins/building-secure-contracts/.claude-plugin/plugin.json index 0da9ba19..87374d7c 100644 --- a/plugins/building-secure-contracts/.claude-plugin/plugin.json +++ b/plugins/building-secure-contracts/.claude-plugin/plugin.json @@ -1,9 +1,9 @@ { "name": "building-secure-contracts", - "version": "1.0.1", + "version": "1.1.0", "description": "Comprehensive smart contract security toolkit based on Trail of Bits' Building Secure Contracts framework. Includes vulnerability scanners for 6 blockchains and 5 development guideline assistants.", "author": { - "name": "Omar Inuwa", + "name": "Omar Inuwa && Paweł Płatek", "email": "opensource@trailofbits.com", "url": "https://github.com/trailofbits" } diff --git a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/CHANGELOG.md b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/CHANGELOG.md new file mode 100644 index 00000000..141ce3cf --- /dev/null +++ b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/CHANGELOG.md @@ -0,0 +1,224 @@ +# Cosmos Vulnerability Scanner — Update Log + +## Third Update (2026-03-18) — 8 Missing Bug Classes Added + +### Summary +Added 8 missing vulnerability classes identified from real-world Cosmos findings catalog. Skill now covers 28 VULNERABILITY_PATTERNS + 16 IBC patterns. No existing patterns were duplicated. + +### New Patterns in VULNERABILITY_PATTERNS.md +- **§20 expanded**: ROUNDING ERRORS → ARITHMETIC ERRORS. Added wrapping overflow (CWA-2024-002), wrong operand (Osmosis $5M), negative value validation (Barberry vesting poisoning) +- **§22 expanded**: Added CacheContext event leak pattern (Huckleberry — events persist after state rollback, enables bridge spoofing) +- **§24 expanded**: EVMOS/ETHERMINT-SPECIFIC → EVM/COSMOS STATE DESYNC. Full precompile atomicity section with 6 detection patterns from 7 findings ($7M+ real losses) +- **§25 new**: STORAGE KEY DESIGN FLAWS — string concatenation collisions, prefix iterator malleability, redelegation bypass +- **§26 new**: CONSENSUS VALIDATION GAPS — vote extension forgery (SEDA), block timestamp manipulation (CometBFT Tachyon), vesting on blocked addresses +- **§27 new**: CIRCUIT BREAKER AUTHZ BYPASS — x/circuit scope escalation +- **§28 new**: MERKLE PROOF / CRYPTOGRAPHIC VERIFICATION FLAWS — ICS-23 proof forgery (Dragonberry), IAVL RangeProof (Dragonfruit/$566M), ECDSA malleability (Gravity Bridge) + +### New Pattern in IBC_VULNERABILITY_PATTERNS.md +- **§16 new**: IBC REENTRANCY / CEI VIOLATIONS — OnTimeoutPacket reentrancy (ASA-2024-007, $150M+ at risk), Terra IBC hooks ($6.4M stolen) + +### SKILL.md Updates +- Updated description, Step 2 checklist, IBC reference count, and Priority Guidelines + +--- + +## Second Update (2026-03-06) — v0.53.x Re-validation + 4 New Patterns + +### Summary +Re-validated all 9 existing patterns against cosmos-sdk v0.53.x (commit `82fcb05ceb`, CometBFT v0.38.21, `cosmossdk.io/math` v1.5.3, `cosmossdk.io/collections` v1.3.1). Added 4 new vulnerability patterns discovered during verification. Skill now covers 13 patterns. + +### Existing Pattern Corrections + +**Pattern 4.4 (Slow ABCI Methods)**: +- Added `HasABCIEndBlock` (returns `[]abci.ValidatorUpdate, error`) as a second EndBlock interface alongside `HasEndBlocker` (returns `error`). Used by x/staking. The skill previously only mentioned one variant. +- Added `HasPrecommit` and `HasPrepareCheckState` as additional lifecycle hooks (confirmed in `core/appmodule/module.go:42-53`). These run after EndBlock and are potential DoS surfaces. + +**Pattern 4.5 (ABCI Panics)**: +- Added note about `ValidateBasic()` deprecation (`types/tx_msg.go:102`: "deprecated and now facultative"). This increases the likelihood of invalid inputs reaching panic-prone code in ABCI methods. + +### New Patterns Added + +**4.10 Unbounded Pagination / Query DoS** (HIGH): +- `types/query/pagination.go` `DefaultLimit=100` only applies when `Limit==0`. Any non-zero client-supplied limit is accepted without cap. +- Recent fix `d9d77304fd` partially addressed this in `x/auth/tx` and `x/bank/keeper/genesis.go`, but core pagination still lacks a hard maximum. + +**4.11 Event Override / Suppression** (MEDIUM): +- New `EventManager.OverrideEvents()` method added in commit `2ddb9ac0a9` (`types/events.go:63`). Replaces all previously emitted events. +- If called in module code, can silently suppress events from auth, bank, IBC — breaking indexers and relayers. + +**4.12 Unordered Transaction Replay** (HIGH): +- SDK v0.53 supports `TxBody.Unordered=true` with timestamp-based replay protection. +- Configurable via `WithMaxUnorderedTxTimeoutDuration` (default 10min) and `WithUnorderedTxGasCost` (default 2240). +- `x/auth` PreBlocker handles `RemoveExpiredUnorderedNonces` cleanup. +- Risks: insufficient timeout, nonce flooding DoS, ordering assumptions in app logic. + +**4.13 Missing msg_server Validation** (HIGH): +- `ValidateBasic()` is now deprecated and facultative (`types/tx_msg.go:102`). +- Modules migrating away from `ValidateBasic` may forget to add equivalent validation in `msg_server.go` handlers. +- Authority/signer checks must be in the handler, not just in the removed `ValidateBasic`. + +### Verification Notes + +All patterns confirmed against the live codebase: +- `sdk.Msg = proto.Message` (types/tx_msg.go:19) — confirmed +- 0 `GetSigners() []AccAddress` methods in x/ — confirmed (only `LegacyMsg` interface defines it) +- 74 `cosmos.msg.v1.signer` annotations across 22 proto files — confirmed +- `appmodule.HasBeginBlocker`, `HasEndBlocker`, `HasPreBlocker` in core/appmodule/module.go:65-89 — confirmed +- `module.HasABCIEndBlock` in types/module/module.go:237-241 — confirmed (different return type!) +- `appmodule.HasPrecommit`, `HasPrepareCheckState` in core/appmodule/module.go:42-53 — confirmed +- `x/auth/module.go` and `x/upgrade/module.go` implement `PreBlock` — confirmed +- `time.Now()` only in test files under x/, not consensus code — confirmed +- `go func` only in generated `pb.gw.go` files under x/ — confirmed +- `blockedAddrs` mechanism in x/bank/keeper/keeper.go — confirmed +- `QuoTruncate`/`QuoRoundUp` on `math.LegacyDec` in math/legacy_dec.go — confirmed +- No handler.go files for legacy SDK handler pattern under x/ — confirmed +- 22 modules under x/ (no x/accounts in this version) — confirmed +- `collections` framework used extensively (81+ occurrences in keeper.go files) — confirmed + +--- + +# First Update (2026-03-06) + +## Context + +The skill's vulnerability patterns were written for Cosmos SDK ~v0.46 and had not been updated for the breaking changes in v0.47–v0.53. The current cosmos-sdk codebase (v0.53.x, Go 1.23, `cosmossdk.io/math` v1.5.3) was used as the reference for validation. + +--- + +## 1. Mismatched Pattern Summary in SKILL.md + +**Problem**: [SKILL.md:98–109](SKILL.md) Section 5 listed 9 CosmWasm-specific patterns (Missing Denom Validation, Reentrancy via Submessages, etc.) that did not correspond to the actual 9 Cosmos SDK patterns defined in [VULNERABILITY_PATTERNS.md](resources/VULNERABILITY_PATTERNS.md). The summary and the resource file described completely different vulnerability classes. + +**Fix**: Replaced the summary to accurately reflect the 9 patterns in the resource file. + +--- + +## 2. Pattern 4.1 — GetSigners: Rewritten for Proto Annotations + +**Problem**: The pattern described `GetSigners() []sdk.AccAddress` as a method on message types. This interface was removed in SDK v0.47. In the current codebase: + +- `sdk.Msg` is just `proto.Message` — [types/tx_msg.go:19](cosmos-sdk/types/tx_msg.go) +- No `func.*GetSigners.*[]sdk.AccAddress` methods exist anywhere in `x/` +- Signers are declared via `cosmos.msg.v1.signer` proto annotations — e.g. [proto/cosmos/gov/v1/tx.proto:60](cosmos-sdk/proto/cosmos/gov/v1/tx.proto) +- Resolution happens in `x/tx/signing` — [x/tx/signing/context.go:358](cosmos-sdk/x/tx/signing/context.go) (`func (c *Context) GetSigners`) + +**Fix**: Rewrote pattern to describe the modern vulnerability: mismatch between `cosmos.msg.v1.signer` proto annotation and the field actually used for authorization in `msg_server.go`. Added detection patterns for wrong field annotation and proto vs handler mismatch. + +--- + +## 3. Pattern 4.2 — Non-Determinism: Math Type References Updated + +**Problem**: Pattern referenced `sdk.Int`, `sdk.Dec`, `sdk.NewDec()` which are removed/deprecated. The codebase uses: + +- `math.Int` / `math.NewInt()` from `cosmossdk.io/math` — 186+ occurrences across `x/` modules +- `math.LegacyDec` / `math.LegacyNewDec()` — 83+ occurrences across `x/` modules +- Zero occurrences of `sdk.NewInt()` or `sdk.NewDec()` in `x/` Go source (only in legacy tests via type aliases) + +**Fix**: Updated all type references: `sdk.Int` → `math.Int`, `sdk.Dec` → `math.LegacyDec`. + +--- + +## 4. Pattern 4.3 — Message Priority: Updated for ABCI 2.0 + +**Problem**: Pattern only discussed `CheckTx` priority. SDK v0.50 introduced ABCI 2.0 with `PrepareProposal` and `ProcessProposal` (267 occurrences in `baseapp/`), which give the block proposer direct control over transaction ordering. + +**Fix**: Added `PrepareProposal`/`ProcessProposal` as the primary mechanism for transaction priority. Updated code examples. + +**Evidence**: [baseapp/abci.go](cosmos-sdk/baseapp/abci.go) — `FinalizeBlock`, `PrepareProposal`, `ProcessProposal` are the ABCI 2.0 entry points. `DeliverTx` still exists internally but is called within `FinalizeBlock`. + +--- + +## 5. Pattern 4.4 — Slow ABCI Methods: Updated Interfaces + +**Problem**: Pattern used standalone `BeginBlocker(ctx sdk.Context, k keeper.Keeper)` function signatures. In SDK v0.50+, modules implement interfaces: + +- `appmodule.HasBeginBlocker` — method `BeginBlock(context.Context) error` +- `appmodule.HasEndBlocker` — method `EndBlock(context.Context) error` +- Module manager calls these via [types/module/module.go:776–825](cosmos-sdk/types/module/module.go) +- `PreBlocker` is new (40 occurrences in codebase) + +**Evidence**: +``` +x/mint/module.go: _ appmodule.HasBeginBlocker = AppModule{} +x/staking/module.go: _ appmodule.HasBeginBlocker = AppModule{} +x/gov/module.go: _ appmodule.HasEndBlocker = AppModule{} +x/feegrant/module.go: _ appmodule.HasEndBlocker = AppModule{} +``` + +**Fix**: Updated all code examples to use `func (am AppModule) BeginBlock(ctx context.Context) error` and `EndBlock` signatures. Added `PreBlock` as a new attack surface. Updated checklist. + +--- + +## 6. Pattern 4.5 — ABCI Panics: Updated Types and Deprecated Params + +**Problem**: Two sub-issues: + +1. **Math types**: Pattern used `sdk.NewDec()`, `sdk.NewInt()`. Current SDK uses `math.NewInt()`, `math.LegacyNewDecFromStr()`, and panic-prone `Must*` variants like `math.LegacyMustNewDecFromStr()`. + +2. **SetParamSet**: Pattern described `SetParamSet` panics. The entire `x/params` module is deprecated: + - [x/params/module.go:61](cosmos-sdk/x/params/module.go): `"Deprecated: the params module is deprecated and will be removed in the next Cosmos SDK major release."` + - [x/params/keeper/keeper.go:15](cosmos-sdk/x/params/keeper/keeper.go): keeper also deprecated + - Modern modules store params directly in keeper state via `collections` + +**Fix**: Updated math type references. Replaced `SetParamSet` panic pattern with direct keeper param storage pattern. Added `Must*` variant warning as a new detection pattern. + +--- + +## 7. Pattern 4.7 — Rounding Errors: Updated Types + +**Problem**: All references used `sdk.Dec`, `sdk.NewDec()`, `sdk.ZeroDec()`. These are gone. + +**Evidence**: `QuoTruncate` and `QuoRoundUp` still exist on `math.LegacyDec` — [math/legacy_dec.go](cosmos-sdk/math/legacy_dec.go) (5 occurrences of QuoTruncate/QuoCeil in codebase). The vulnerability pattern itself is still valid. + +**Fix**: Updated all type references to `math.LegacyDec`, `math.LegacyNewDec()`, `math.LegacyZeroDec()`. + +--- + +## 8. Pattern 4.8 — Unregistered Handler: Marked Legacy Only + +**Problem**: Pattern described `NewHandler` / `sdk.Handler` switch-case pattern. This is completely gone from the SDK: + +- Zero `handler.go` files exist under `x/` +- Zero matches for `NewHandler` or `sdk.Handler` in Go files under `x/` +- All 16 modules use `msg_server.go` with protobuf service registration: + ``` + x/auth/keeper/msg_server.go + x/bank/keeper/msg_server.go + x/gov/keeper/msg_server.go + x/staking/keeper/msg_server.go + ... (16 total) + ``` + +**Fix**: Marked pattern as legacy-only (pre-v0.47). Added modern equivalent: stub/no-op `msg_server.go` implementations where a proto RPC method is technically implemented but does nothing. The Go compiler catches missing methods (unlike the old switch-case), but stub implementations are still a risk. + +--- + +## 9. Scanning Workflow: Updated Throughout + +**Changes in [SKILL.md](SKILL.md) Section 6**: + +| Old | New | +|-----|-----| +| `handler.go` — Message handlers (legacy) | `keeper/msg_server.go` — Message handlers (protobuf service) | +| `abci.go` — BeginBlocker/EndBlocker | `module.go` — appmodule interface implementations | +| `types/msgs.go`, `GetSigners()` | `proto/.../tx.proto` with `cosmos.msg.v1.signer` annotations | +| `DeliverTx` | `FinalizeBlock` | +| `CheckTx` priority only | `PrepareProposal` / `ProcessProposal` / `CheckTx` | +| `sdk.Dec` / `sdk.Int` | `math.LegacyDec` / `math.Int` | + +**Quick Reference Checklist** updated similarly — signer check now references proto annotations, ABCI check includes PreBlock, testing uses `FinalizeBlock`. + +--- + +## Files Modified + +- [SKILL.md](SKILL.md) — Full rewrite of sections 3, 5, 6, 7, 9, 10, 11 +- [resources/VULNERABILITY_PATTERNS.md](resources/VULNERABILITY_PATTERNS.md) — All 9 patterns updated + +## Validation Method + +Each pattern was validated against the cosmos-sdk codebase against `cosmos-sdk` (v0.53.x, commit `82fcb05ceb`) using grep/glob searches to confirm: +- Which interfaces/types still exist +- Which have been removed or deprecated +- What replaced them +- Occurrence counts to verify patterns are real diff --git a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/SKILL.md b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/SKILL.md index 61b55ed8..840a45d2 100644 --- a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/SKILL.md +++ b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/SKILL.md @@ -1,334 +1,188 @@ --- name: cosmos-vulnerability-scanner -description: Scans Cosmos SDK blockchains for 9 consensus-critical vulnerabilities including non-determinism, incorrect signers, ABCI panics, and rounding errors. Use when auditing Cosmos chains or CosmWasm contracts. +description: "Scans Cosmos SDK blockchain modules and CosmWasm contracts for consensus-critical vulnerabilities — chain halts, fund loss, state divergence. 25 core + 16 IBC + 10 EVM + 3 CosmWasm patterns. Use when auditing custom x/ modules, reviewing IBC integrations, or assessing pre-launch chain security. Updated for SDK v0.53.x." --- # Cosmos Vulnerability Scanner -## 1. Purpose +## Purpose -Systematically scan Cosmos SDK blockchain modules and CosmWasm smart contracts for platform-specific security vulnerabilities that can cause chain halts, consensus failures, or fund loss. This skill encodes 9 critical vulnerability patterns unique to Cosmos-based chains. +Scan Cosmos SDK modules and CosmWasm contracts for vulnerabilities that cause chain halts, consensus failures, or fund loss. Spawns parallel scanning agents — each specializing in a vulnerability category — that return findings to the main skill, which then writes them as individual markdown files to an output directory. -## 2. When to Use This Skill +**Output directory**: defaults to `.bughunt_cosmos/`. If the user specifies a different directory in their prompt, use that instead. -- Auditing Cosmos SDK modules (custom x/ modules) -- Reviewing CosmWasm smart contracts (Rust) +## When to Use + +- Auditing Cosmos SDK modules (custom `x/` modules) +- Reviewing CosmWasm smart contracts - Pre-launch security assessment of Cosmos chains - Investigating chain halt incidents -- Validating consensus-critical code changes -- Reviewing ABCI method implementations - -## 3. Platform Detection - -### File Extensions & Indicators -- **Go files**: `.go`, `.proto` -- **CosmWasm**: `.rs` (Rust with cosmwasm imports) - -### Language/Framework Markers -```go -// Cosmos SDK indicators -import ( - "github.com/cosmos/cosmos-sdk/types" - sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/x/..." -) - -// Common patterns -keeper.Keeper -sdk.Msg, GetSigners() -BeginBlocker, EndBlocker -CheckTx, DeliverTx -protobuf service definitions -``` - -```rust -// CosmWasm indicators -use cosmwasm_std::*; -#[entry_point] -pub fn execute(deps: DepsMut, env: Env, info: MessageInfo, msg: ExecuteMsg) -``` -### Project Structure -- `x/modulename/` - Custom modules -- `keeper/keeper.go` - State management -- `types/msgs.go` - Message definitions -- `abci.go` - BeginBlocker/EndBlocker -- `handler.go` - Message handlers (legacy) +## When NOT to Use -### Tool Support -- **CodeQL**: Custom rules for non-determinism and panics -- **go vet**, **golangci-lint**: Basic Go static analysis -- **Manual review**: Critical for consensus issues +- Pure Solidity/EVM audits without Cosmos SDK — use Solidity-specific tools +- CometBFT consensus engine internals — this covers SDK modules, not the consensus layer itself +- General Go code review with no blockchain context +- Cosmos SDK application logic that is not consensus-critical (e.g., CLI commands, REST endpoints) +- CosmWasm contract-only audits on chains without custom SDK modules — use the CosmWasm checklist items alone ---- +## Essential Principles -## 4. How This Skill Works +1. **Consensus path is king** — A bug only matters for chain halt/fund loss if it's on the consensus-critical execution path (BeginBlock, EndBlock, FinalizeBlock, msg_server handlers, AnteHandler). Always verify a finding is reachable from consensus before reporting it. +2. **State divergence = chain halt** — Any non-determinism that causes validators to compute different state roots will halt the chain. This is the highest-severity class because it affects all validators simultaneously. +3. **Check the version** — Cosmos SDK has breaking changes across major versions (v0.47 removed GetSigners, v0.50 added ABCI 2.0, v0.53 deprecated ValidateBasic). Always check `go.mod` versions before applying patterns. +4. **False positives waste audit time** — A map iteration in a CLI command is not a consensus bug. A panic in a query handler does not halt the chain. Verify the execution context before flagging. +5. **Cross-module interactions are where bugs hide** — The most severe findings (IBC reentrancy, EVM/Cosmos state desync, authz escalation) involve interactions between modules, not bugs within a single module. -When invoked, I will: +## Scanning Workflow -1. **Search your codebase** for Cosmos SDK modules -2. **Analyze each module** for the 9 vulnerability patterns -3. **Report findings** with file references and severity -4. **Provide fixes** for each identified issue -5. **Check message handlers** for validation issues +### Phase 1: Discovery (synchronous) ---- +**Entry**: Target codebase path provided by user. Codebase contains Go source (e.g., `x/` modules, `go.mod`) or Rust contracts with `cosmwasm_std`. -## 5. Example Output +Run a **synchronous subagent** (Agent tool) with the full contents of [DISCOVERY.md](resources/DISCOVERY.md) as its prompt. The agent must: -When vulnerabilities are found, you'll get a report like this: +1. Follow the Discovery workflow to explore the target codebase +2. Return the full CLAUDE.md content (the technical inventory and threat model) in its response +3. Return a structured summary with exactly these fields: ``` -=== COSMOS SDK VULNERABILITY SCAN RESULTS === +PLATFORM: pure-cosmos | evm | wasm (pick one; if multiple, comma-separated) +IBC_ENABLED: true | false +SDK_VERSION: +IBC_GO_VERSION: +CUSTOM_MODULES: +``` -Project: my-cosmos-chain -Files Scanned: 6 (.go) -Vulnerabilities Found: 2 +After the subagent returns, **you** (the main skill) Write the CLAUDE.md to the target repo root. Save its path and the discovery values — these feed into Phase 2. ---- +**Exit**: CLAUDE.md written by main skill. PLATFORM, IBC_ENABLED, SDK_VERSION, IBC_GO_VERSION, and CUSTOM_MODULES captured. -[CRITICAL] Incorrect GetSigners() +### Phase 2: Parallel Vulnerability Scan ---- +Spawn scanning agents **in a single message** for maximum parallelism. Use the Agent Prompt Template below, filling in the reference file for each agent. Subagents only need read access (Grep, Glob, Read) — they return findings in their response and the main skill writes the files. -## 5. Vulnerability Patterns (9 Patterns) - -I check for 9 critical vulnerability patterns unique to CosmWasm. For detailed detection patterns, code examples, mitigations, and testing strategies, see [VULNERABILITY_PATTERNS.md](resources/VULNERABILITY_PATTERNS.md). - -### Pattern Summary: - -1. **Missing Denom Validation** ⚠️ CRITICAL - Accepting arbitrary token denoms -2. **Insufficient Authorization** ⚠️ CRITICAL - Missing sender/admin validation -3. **Missing Balance Check** ⚠️ HIGH - Not verifying sufficient balances -4. **Improper Reply Handling** ⚠️ HIGH - Unsafe submessage reply processing -5. **Missing Reply ID Check** ⚠️ MEDIUM - Not validating reply IDs -6. **Improper IBC Packet Validation** ⚠️ CRITICAL - Unvalidated IBC packets -7. **Unvalidated Execute Message** ⚠️ HIGH - Missing message validation -8. **Integer Overflow** ⚠️ HIGH - Unchecked arithmetic operations -9. **Reentrancy via Submessages** ⚠️ MEDIUM - State changes before submessages - -For complete vulnerability patterns with code examples, see [VULNERABILITY_PATTERNS.md](resources/VULNERABILITY_PATTERNS.md). -## 5. Scanning Workflow - -### Step 1: Platform Identification -1. Identify Cosmos SDK version (`go.mod`) -2. Locate custom modules (`x/*/`) -3. Find ABCI methods (`abci.go`, BeginBlocker, EndBlocker) -4. Identify message types (`types/msgs.go`, `.proto`) - -### Step 2: Critical Path Analysis -Focus on consensus-critical code: -- BeginBlocker / EndBlocker implementations -- Message handlers (execute, DeliverTx) -- Keeper methods that modify state -- CheckTx priority logic - -### Step 3: Non-Determinism Sweep -**This is the highest priority check for Cosmos chains.** - -```bash -# Search for non-deterministic patterns -grep -r "range.*map\[" x/ -grep -r "\bint\b\|\buint\b" x/ | grep -v "int32\|int64\|uint32\|uint64" -grep -r "float32\|float64" x/ -grep -r "go func\|go routine" x/ -grep -r "select {" x/ -grep -r "time.Now()" x/ -grep -r "rand\." x/ -``` +**Always spawn these 3 agents:** -For each finding: -1. Verify it's in consensus-critical path -2. Confirm it causes non-determinism -3. Assess severity (chain halt vs data inconsistency) - -### Step 4: ABCI Method Analysis -Review BeginBlocker and EndBlocker: -- [ ] Computational complexity bounded? -- [ ] No unbounded iterations? -- [ ] No nested loops over large collections? -- [ ] Panic-prone operations validated? -- [ ] Benchmarked with maximum state? - -### Step 5: Message Validation -For each message type: -- [ ] GetSigners() address matches handler usage? -- [ ] All error returns checked? -- [ ] Priority set in CheckTx if critical? -- [ ] Handler registered (or using v0.47+ auto-registration)? - -### Step 6: Arithmetic & Bookkeeping -- [ ] sdk.Dec operations use multiply-before-divide? -- [ ] Rounding favors protocol over users? -- [ ] Custom bookkeeping synchronized with x/bank? -- [ ] Invariant checks in place? +| Agent Name | Reference File | Scope | +|------------|---------------|-------| +| `core-scanner` | `VULNERABILITY_PATTERNS.md` | §1-9: non-determinism, ABCI, signers, validation, handlers, ante security | +| `state-scanner` | `STATE_VULNERABILITY_PATTERNS.md` | §11-23: bookkeeping, bank, pagination, events, tx replay, governance, arithmetic, encoding, deprecated modules | +| `advanced-scanner` | `ADVANCED_VULNERABILITY_PATTERNS.md` | §24-27: storage keys, consensus validation, circuit breaker, crypto | ---- +**Spawn conditionally (in the same parallel message):** -## 6. Reporting Format +| Agent Name | Condition | Reference File | +|------------|-----------|---------------| +| `evm-scanner` | PLATFORM includes `evm` | `EVM_VULNERABILITY_PATTERNS.md` | +| `ibc-scanner` | IBC_ENABLED is `true` | `IBC_VULNERABILITY_PATTERNS.md` | +| `cosmwasm-scanner` | PLATFORM includes `wasm` | `COSMWASM_VULNERABILITY_PATTERNS.md` | -### Finding Template -```markdown -## [CRITICAL] Non-Deterministic Map Iteration in EndBlocker +#### Agent Prompt Template -**Location**: `x/dex/abci.go:45-52` +Construct each agent's prompt by replacing `{REFERENCE_FILE_PATH}` with the full path to the reference file (under `{baseDir}/resources/`) and `{CLAUDE_MD_PATH}` with the path to the CLAUDE.md written in Phase 1: -**Description**: -The EndBlocker iterates over an unordered map to distribute rewards, causing different validators to process users in different orders and produce different state roots. This will halt the chain when validators fail to reach consensus. +~~~ +Perform a very thorough security scan of a Cosmos SDK codebase for specific vulnerability patterns. -**Vulnerable Code**: -```go -// abci.go, line 45 -func EndBlocker(ctx sdk.Context, k keeper.Keeper) { - rewards := k.GetPendingRewards(ctx) // Returns map[string]sdk.Coins - for user, amount := range rewards { // NON-DETERMINISTIC ORDER - k.bankKeeper.SendCoins(ctx, moduleAcc, user, amount) - } -} -``` +CONTEXT: +Read {CLAUDE_MD_PATH} for codebase context (SDK version, modules, threat model, key files). -**Attack Scenario**: -1. Multiple users have pending rewards -2. Different validators iterate in different orders due to map randomization -3. If any reward distribution fails mid-iteration, state diverges -4. Validators produce different app hashes -5. Chain halts - cannot reach consensus - -**Recommendation**: -Sort map keys before iteration: -```go -func EndBlocker(ctx sdk.Context, k keeper.Keeper) { - rewards := k.GetPendingRewards(ctx) - - // Collect and sort keys for deterministic iteration - users := make([]string, 0, len(rewards)) - for user := range rewards { - users = append(users, user) - } - sort.Strings(users) // Deterministic order - - // Process in sorted order - for _, user := range users { - k.bankKeeper.SendCoins(ctx, moduleAcc, user, rewards[user]) - } -} -``` +PATTERNS: +Read {REFERENCE_FILE_PATH} — it contains numbered vulnerability patterns. For EACH pattern: +1. Read the detection patterns and "What to Check" items +2. Use Grep and Glob to search the target codebase for each pattern +3. When a match is found, Read surrounding code to verify it's on a consensus-critical path (BeginBlock, EndBlock, FinalizeBlock, msg_server handlers, AnteHandler) +4. Classify severity per the guidelines below -**References**: -- building-secure-contracts/not-so-smart-contracts/cosmos/non_determinism -- Cosmos SDK docs: Determinism -``` +RULES: +- Consensus path only: Only flag code reachable from consensus-critical execution. CLI/query/test code is NOT a finding. +- Check SDK version in go.mod before applying patterns (v0.47 removed GetSigners, v0.50 added ABCI 2.0, v0.53 deprecated ValidateBasic). +- Always use the Grep tool for searches, not bash grep. The reference file contains search patterns — use them directly with the Grep tool. +- Ignore cross-references to other resource files (e.g., links to IBC or COSMWASM patterns). Those patterns are covered by other scanning agents. +- Reject these rationalizations: + - "ValidateBasic catches this" — deprecated and facultative since SDK v0.53 + - "Behind governance, so safe" — governance proposals can be malicious + - "IBC counterparty is trusted" — any chain can open a channel + - "Panic can't happen, input is validated" — trace the full call chain + - "Rounding error is only a few tokens" — compounds over time, can be looped + - "EVM precompile handles rollback" — many have incomplete rollback ---- +SEVERITY: +- Critical (fund loss): signer mismatch, broken bookkeeping, AnteHandler bypass, bank keeper misuse, IBC token inflation, EVM/Cosmos desync, Merkle proof forgery, arithmetic overflow +- High (chain halt): non-determinism, ABCI panics, slow ABCI, non-deterministic IBC acks, consensus gaps, CacheContext event leak +- Medium (DoS): unbounded pagination, tx replay, missing validation, governance spam, rate limiting, circuit breaker bypass, storage key collisions +- Low (logic): rounding errors, stub handlers, event override, module ordering -## 7. Priority Guidelines +OUTPUT — RETURN FORMAT: +Do NOT write any files. Return ALL findings and the summary in your response. -### Critical - CHAIN HALT Risk -- Non-determinism (any form) -- ABCI method panics -- Slow ABCI methods -- Incorrect GetSigners (allows unauthorized actions) +For each pattern, return one of: + §NUM PATTERN_NAME: Not applicable — [one-line reason] + §NUM PATTERN_NAME: FINDING (followed by the finding block below) -### High - Fund Loss Risk -- Missing error handling (bankKeeper.SendCoins) -- Broken bookkeeping (accounting mismatch) -- Missing message priority (oracle/emergency messages) +For each finding, include the full content using this template: -### Medium - Logic/DoS Risk -- Rounding errors (protocol value leakage) -- Unregistered message handlers (functionality broken) +FINDING_FILE: {SEVERITY}-s{SECTION_NUM}-{kebab-description}.md +## [SEVERITY] Title +**Location**: `file:line` +**Description**: What the bug is and why it matters +**Vulnerable Code**: [snippet] +**Attack Scenario**: [numbered steps] +**Recommendation**: How to fix +**References**: [links to relevant advisories or building-secure-contracts] ---- +You MUST report on ALL patterns in the reference file — do not skip any. +~~~ -## 8. Testing Recommendations +**Exit**: All scanning agents returned. Each reported on every pattern in their reference file. -### Non-Determinism Testing -```bash -# Build for different architectures -GOARCH=amd64 go build -GOARCH=arm64 go build +### Phase 3: Write Findings -# Run same operations, compare state roots -# Must be identical across architectures +After all scanning agents return, write finding files to the output directory (default `.bughunt_cosmos/`): -# Fuzz test with concurrent operations -go test -fuzz=FuzzEndBlocker -parallel=10 -``` +1. Parse each agent's response for `FINDING_FILE:` blocks +2. For each finding, Write the content to `{OUTPUT_DIR}/{filename}` using the filename from `FINDING_FILE:` +3. Create the output directory first if it doesn't exist -### ABCI Benchmarking -```go -func BenchmarkBeginBlocker(b *testing.B) { - ctx := setupMaximalState() // Worst-case state - b.ResetTimer() +### Phase 4: Verify Completeness - for i := 0; i < b.N; i++ { - BeginBlocker(ctx, keeper) - } +After writing all findings, verify every pattern was assessed: - // Must complete in < 1 second - require.Less(b, b.Elapsed()/time.Duration(b.N), time.Second) -} -``` +1. Collect the summary lines (§NUM entries) returned by each agent +2. Check pattern counts against expected totals: + - `core-scanner`: 8 patterns (§1-9, excluding §8 legacy-only) + - `state-scanner`: 13 patterns (§11-23) + - `advanced-scanner`: 4 patterns (§24-27) + - `evm-scanner` (if spawned): 10 patterns (§1-10) + - `ibc-scanner` (if spawned): 16 patterns (§1-16) + - `cosmwasm-scanner` (if spawned): 3 patterns (§1-3) +3. If any pattern is missing from a summary, flag it and re-prompt that agent +4. List all finding files written to the output directory with a `Glob` for `*.md` -### Invariant Testing -```go -// Run invariants in integration tests -func TestInvariants(t *testing.T) { - app := setupApp() +**Exit**: All patterns accounted for. Finding files listed for the user. - // Execute operations - app.DeliverTx(...) +--- - // Check invariants - _, broken := keeper.AllInvariants()(app.Ctx) - require.False(t, broken, "invariant violation detected") -} -``` +## Success Criteria + +- [ ] Discovery CLAUDE.md written with complete technical inventory and threat model +- [ ] All scanning agents completed and reported on every pattern in their reference file +- [ ] Pattern counts verified against expected totals (no patterns skipped) +- [ ] All findings written to output directory as individual markdown files +- [ ] Each finding file includes: severity, location, vulnerable code, attack scenario, recommendation --- -## 9. Additional Resources +## Resources +- **Discovery & CLAUDE.md**: [DISCOVERY.md](resources/DISCOVERY.md) +- **Core patterns (§1-9)**: [VULNERABILITY_PATTERNS.md](resources/VULNERABILITY_PATTERNS.md) +- **State & module patterns (§11-23)**: [STATE_VULNERABILITY_PATTERNS.md](resources/STATE_VULNERABILITY_PATTERNS.md) +- **Advanced patterns (§24-27)**: [ADVANCED_VULNERABILITY_PATTERNS.md](resources/ADVANCED_VULNERABILITY_PATTERNS.md) +- **IBC vulnerabilities**: [IBC_VULNERABILITY_PATTERNS.md](resources/IBC_VULNERABILITY_PATTERNS.md) +- **CosmWasm vulnerabilities**: [COSMWASM_VULNERABILITY_PATTERNS.md](resources/COSMWASM_VULNERABILITY_PATTERNS.md) +- **EVM vulnerabilities**: [EVM_VULNERABILITY_PATTERNS.md](resources/EVM_VULNERABILITY_PATTERNS.md) - **Building Secure Contracts**: `building-secure-contracts/not-so-smart-contracts/cosmos/` - **Cosmos SDK Docs**: https://docs.cosmos.network/ -- **CodeQL for Go**: https://codeql.github.com/docs/codeql-language-guides/codeql-for-go/ -- **Cosmos Security Best Practices**: https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/learn/advanced/17-determinism.md - ---- - -## 10. Quick Reference Checklist - -Before completing Cosmos chain audit: - -**Non-Determinism (CRITICAL)**: -- [ ] No map iteration in consensus code -- [ ] No platform-dependent types (int, uint, float) -- [ ] No goroutines in message handlers/ABCI -- [ ] No select statements with multiple channels -- [ ] No rand, time.Now(), memory addresses -- [ ] All serialization is deterministic - -**ABCI Methods (CRITICAL)**: -- [ ] BeginBlocker/EndBlocker computationally bounded -- [ ] No unbounded iterations -- [ ] No nested loops over large collections -- [ ] All panic-prone operations validated -- [ ] Benchmarked with maximum state - -**Message Handling (HIGH)**: -- [ ] GetSigners() matches handler address usage -- [ ] All error returns checked -- [ ] Critical messages prioritized in CheckTx -- [ ] All message types registered - -**Arithmetic & Accounting (MEDIUM)**: -- [ ] Multiply before divide pattern used -- [ ] Rounding favors protocol -- [ ] Custom bookkeeping synced with x/bank -- [ ] Invariant checks implemented - -**Testing**: -- [ ] Cross-architecture builds tested -- [ ] ABCI methods benchmarked -- [ ] Invariants checked in CI -- [ ] Integration tests cover all messages +- **CodeQL for Cosmos SDK**: https://github.com/crypto-com/cosmos-sdk-codeql diff --git a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/ADVANCED_VULNERABILITY_PATTERNS.md b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/ADVANCED_VULNERABILITY_PATTERNS.md new file mode 100644 index 00000000..caec1c88 --- /dev/null +++ b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/ADVANCED_VULNERABILITY_PATTERNS.md @@ -0,0 +1,236 @@ +## Advanced Vulnerability Patterns + +> Updated for Cosmos SDK v0.53.x. Advanced infrastructure patterns (§24-27) covering storage key design, consensus validation gaps, circuit breaker authorization, and cryptographic verification. Referenced by [SKILL.md](../SKILL.md) checklist items via `§` anchors. + +### 24 STORAGE KEY DESIGN FLAWS + +**Description**: KV store key construction allows collisions, prefix overlaps, or unauthorized access to other users' data. Keys built via string concatenation without length-prefixed encoding create ambiguous boundaries. Numeric fields serialized as variable-length strings share byte prefixes. Prefix iterators over attacker-controllable components include unintended objects. + +**Detection Patterns**: + +#### Pattern 1: String Concatenation Collision +```go +// VULNERABLE: String concatenation without length-prefix encoding (Finding 16) +func VaultUserKey(vaultID uint64, user string) []byte { + return []byte(fmt.Sprintf("%d%s", vaultID, user)) +} +// vaultId=1, user="2a" → key "12a" +// vaultId=12, user="a" → key "12a" ← COLLISION! + +// Even with separators, string fields containing the separator collide: +// vaultId=1, user="a|", position="b" → "1|a||b" +// vaultId=1, user="a", position="|b" → "1|a||b" ← COLLISION! +``` + +#### Pattern 2: Prefix Iterator Malleability +```go +// VULNERABLE: Pool IDs serialized as strings in KV store keys (Finding 31) +func PositionKey(poolID uint64, posID uint64) []byte { + return []byte(fmt.Sprintf("position/%d/%d", poolID, posID)) +} +// poolID=42 shares byte prefix [52 50] with poolID=421 +// HasAnyAtPrefix("position/42") matches "position/421/..." +// Attacker creates positions with carefully chosen pool IDs +// that prefix-collide with victim positions + +// VULNERABLE: Attacker can increment pool IDs by creating empty pools +store.Iterator(PositionPrefix(poolID), nil) // Includes poolID * 10 + N +``` + +#### Pattern 3: Redelegation State Lookup Bypass +```go +// VULNERABLE: Unbonding period bypass via redelegation (Finding 14) +// If a user redelegates to a validator outside the active set +// (e.g., bonded stake rank 102), then unbonds, the 21-day +// unbonding period is skipped — the state key lookup for the +// unbonding entry doesn't account for out-of-set validators. +// Attacker who steals tokens can instantly exit staking. +``` + +**What to Check**: +- [ ] KV store keys use length-prefixed encoding (e.g., `collections.PairKeyCodec`, `address.LengthPrefixedAddressKey`) — NOT string concatenation +- [ ] Prefix iterators cannot be confused by attacker-controllable numeric IDs (e.g., pool IDs serialized as fixed-width or length-prefixed) +- [ ] String fields in composite keys cannot contain the separator character +- [ ] `HasAnyAtPrefix` / `Iterator` with user-influenced prefix components verified to not match unintended keys +- [ ] Redelegation to out-of-set validators followed by unbonding is tested against unbonding period enforcement + +**Mitigation**: +```go +// SECURE: Use length-prefixed encoding +func VaultUserKey(vaultID uint64, user string) []byte { + key := make([]byte, 8+1+len(user)) + binary.BigEndian.PutUint64(key[:8], vaultID) // Fixed-width + key[8] = byte(len(user)) // Length prefix + copy(key[9:], user) + return key +} + +// SECURE: Use cosmossdk.io/collections key codecs +var PositionsMap = collections.NewMap( + sb, PositionsPrefix, + collections.PairKeyCodec(collections.Uint64Key, collections.Uint64Key), + codec.CollValue[Position](cdc), +) +``` + +**References**: +- [Zellic — Cosmos Security Primer](https://www.zellic.io/blog/exploring-cosmos-a-security-primer/) (Finding 16) +- [Fault Tolerant — Cosmos Security Handbook](https://www.faulttolerant.xyz/2024-01-16-cosmos-security-1/) (Finding 31) +- [Node A-Team — Unbonding Period Bypass](https://medium.com/node-a-team/cosmos-hub-security-vulnerability-regarding-redelegation-and-unbonding-e6a2cf03e928) (Finding 14) + +--- + +### 25 CONSENSUS VALIDATION GAPS + +**Description**: Missing or incomplete validation at the consensus protocol layer — vote extensions, block timestamp manipulation, and account type checks at creation time. These allow Byzantine validators or front-running attackers to manipulate consensus-level data that downstream modules trust implicitly. + +**Detection Patterns**: + +#### Pattern 1: Vote Extension Forgery +```go +// VULNERABLE: PrepareProposal validates signatures but skips +// VerifyVoteExtension rules (Finding 15, SEDA Chain) +func (h *Handler) PrepareProposal(ctx sdk.Context, req *abci.PrepareProposalRequest) (*abci.PrepareProposalResponse, error) { + for _, vote := range req.LocalLastCommit.Votes { + // Only checks vote signature — does NOT re-run VerifyVoteExtension! + if !verifySignature(vote) { continue } + // CometBFT accepts late precommits without full verification + // Byzantine validator smuggles malicious vote extension into proposal + extensions = append(extensions, vote.VoteExtension) + } + // Leader unknowingly includes forged oracle data in block +} +``` + +#### Pattern 2: Block Timestamp Manipulation +```go +// VULNERABLE: Any module trusting ctx.BlockTime() for security-critical decisions +// CometBFT "Tachyon" (Finding 21, CSA-2026-001): inconsistency between how +// commit signatures are verified and how block time is derived breaks BFT Time +// guarantee. A faulty process CAN arbitrarily increase block time. + +// These are all exploitable if block time can be manipulated: +deadline := proposal.SubmitTime.Add(votingPeriod) +if ctx.BlockTime().After(deadline) { ... } // Governance deadlines + +vestingEnd := account.EndTime +if ctx.BlockTime().After(vestingEnd) { ... } // Vesting unlocks + +price := oracle.GetTWAP(ctx, pair, window) // Time-weighted prices +``` + +#### Pattern 3: Vesting on Blocked/Module Addresses +```go +// VULNERABLE: Creating vesting accounts on blocked addresses (Finding 29, ASA-2024-003) +func (ms msgServer) CreatePeriodicVestingAccount(ctx context.Context, msg *types.MsgCreatePeriodicVestingAccount) error { + toAddr := sdk.AccAddressFromBech32(msg.ToAddress) + // Missing: check if toAddress is a blocked address or module account + // Funds deposited to module accounts via vesting become permanently inaccessible + // Could also enable unintended interactions with module accounts +} +``` + +**What to Check**: +- [ ] `PrepareProposal` re-validates vote extensions using the same rules as `VerifyVoteExtension` — do NOT trust that CometBFT has already validated them (late precommits may skip verification) +- [ ] `ProcessProposal` independently validates all vote extensions included in the proposal +- [ ] Vote extension content is treated as untrusted input even from non-Byzantine validators +- [ ] CometBFT is patched for Tachyon (>= v0.38.21 / v0.37.18) — block timestamp manipulation +- [ ] Time-dependent logic (governance deadlines, vesting unlocks, oracle TWAPs, lockup periods, auction timers) is reviewed against block time manipulation +- [ ] `CreateVestingAccount` / `CreatePeriodicVestingAccount` reject blocked addresses and non-initialized module accounts as recipients +- [ ] SDK version includes Barberry patches (>= v0.46.13, >= v0.47.3) for vesting on blocked addresses + +**References**: +- [OtterSec — SEDA Vote Extension Forgery](https://osec.io/blog/2025-06-10-cosmos-security/) (Finding 15) +- [CometBFT Tachyon CSA-2026-001](https://github.com/cometbft/cometbft/security/advisories/GHSA-c32p-wcqj-j677) (Finding 21) +- [ASA-2024-003 Vesting on Blocked Addresses](https://github.com/cosmos/cosmos-sdk/security/advisories) (Finding 29) + +--- + +### 26 CIRCUIT BREAKER AUTHZ BYPASS + +**Description**: The `x/circuit` module allows authorized accounts to disable (circuit-break) specific message types during incident response. An authorization scope bug allows an account authorized to circuit-break specific message types to reset the circuit breaker for **any** message type, even those it lacks permission for. This can re-enable message types intentionally disabled for security. + +**Detection Patterns**: +```go +// VULNERABLE: Circuit breaker reset does not verify per-type authorization +// Account authorized to break MsgSend can reset MsgDelegate +func (ms msgServer) ResetCircuitBreaker(ctx context.Context, msg *types.MsgResetCircuitBreaker) error { + // Checks: is the sender an authorized circuit breaker account? YES + // Missing: does the sender have authority over THIS SPECIFIC message type? + // Result: any circuit breaker account can re-enable any disabled message type +} +``` + +**What to Check**: +- [ ] `x/circuit` reset/authorize operations validate per-message-type permissions (not just "is this a circuit breaker account") +- [ ] Circuit breaker accounts have minimal scope — only the message types they are responsible for +- [ ] Re-enabling a circuit-broken message type requires the same or higher authorization level as disabling it +- [ ] During incident response, monitor for unexpected circuit breaker resets + +**Grep Patterns** (use Grep tool): +- `circuit|CircuitBreaker|ResetCircuitBreaker` with `glob: "*.go"` in `x/` +- `x/circuit` with `glob: "*.go"` in `app/` + +**References**: +- [HackerOne Report 2120609](https://hackerone.com/reports/2120609) (Finding 28) + +--- + +### 27 MERKLE PROOF / CRYPTOGRAPHIC VERIFICATION FLAWS + +**Description**: Specification-level or implementation-level flaws in cryptographic proof systems allow forging proofs that should be computationally infeasible. ICS-23 was unsound at the spec level (not just implementation). Cross-chain bridges that verify proofs differently on each side create asymmetric validation. This class has caused the largest single loss in the Cosmos ecosystem ($566M BNB bridge exploit, related flaw). + +**Detection Patterns**: + +#### Pattern 1: ICS-23 Absence Proof Forgery +``` +// VULNERABLE: ICS-23 spec allowed proofs to carry their own spec parameters +// (Finding 3a, Dragonberry — all IBC chains, all versions) +// Validation only required input leaf prefix to CONTAIN the standard spec's +// prefix as a substring — not match exactly. +// Attacker forges absence proof to "prove" packet commitment doesn't exist +// on counterparty chain (even though it does), then triggers IBC timeout refunds +// for packets that were successfully delivered. +// Iterative drainage of ALL ICS-20 escrow accounts across ALL Cosmos networks. +``` + +#### Pattern 2: IAVL RangeProof Forgery +``` +// VULNERABLE: Same class as Dragonberry in IAVL RangeProof system +// (Finding 30, Dragonfruit — $566M BNB bridge exploit used related flaw) +// Missing validation checks on leaf/inner-node prefix/suffix length +// allowed forging proofs against the IAVL Merkle tree. +``` + +#### Pattern 3: ECDSA Signature Malleability +```go +// VULNERABLE: Cosmos-side ECDSA verification accepts non-canonical s-values +// (Finding 13, Gravity Bridge) +func EthAddressFromSignature(hash []byte, sig []byte) (common.Address, error) { + // Missing: canonical s-value validation + // ECDSA has two valid s values per signature (elliptic curve symmetry) + // Ethereum enforces s < secp256k1.N/2, Cosmos side does not + // Malicious orchestrator submits non-canonical signature: + // Cosmos accepts → Ethereum rejects → bridge enters crash loop + // Selectively block validator set updates and transaction batches +} +``` + +**What to Check**: +- [ ] ICS-23 proof verification uses patched libraries — leaf prefix must match exactly, not just contain as substring +- [ ] IAVL library patched for Dragonfruit (prefix/suffix length validation on inner/leaf nodes) +- [ ] ECDSA signature verification enforces canonical s-value (`s < secp256k1.N/2`) on BOTH sides of any bridge +- [ ] Cross-chain proof verification is symmetric — same validation on source and destination +- [ ] Custom Merkle proof implementations do not allow proof parameters to be attacker-controlled +- [ ] If using BLS, ed25519, or other signature schemes for cross-chain verification, signature malleability is addressed + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `VerifyMembership|VerifyNonMembership|VerifyProof` — proof verification +- `RangeProof|InnerNode|LeafNode` — Merkle tree operations +- `EthAddressFromSignature|RecoverPubkey|Ecrecover` — signature recovery + +**References**: +- [Dragonberry / ICS-23 Proof Forgery](https://forum.cosmos.network/t/ibc-security-advisory-dragonberry/7702) (Finding 3a) +- [Verichains ICS-23 analysis](https://blog.verichains.io/p/vsa-2022-103-cosmos-sdk-forging-membership) +- [Dragonfruit / IAVL RangeProof](https://forum.cosmos.network/t/cosmos-sdk-security-advisory-dragonfruit/7614) (Finding 30) +- [Gravity Bridge ECDSA malleability](https://maxwelldulin.com/BlogPost/Cryptographic-Asymmetry-and-How-To-Shut-Down-A-CosmosEthereum-Bridge) (Finding 13) diff --git a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/COSMWASM_VULNERABILITY_PATTERNS.md b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/COSMWASM_VULNERABILITY_PATTERNS.md new file mode 100644 index 00000000..65a7b87f --- /dev/null +++ b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/COSMWASM_VULNERABILITY_PATTERNS.md @@ -0,0 +1,145 @@ +## CosmWasm Vulnerability Patterns & Audit Checks + +> Vulnerability patterns specific to CosmWasm smart contracts (Rust contracts on `wasmd`). Referenced by [SKILL.md](../SKILL.md). + +### 1 WRAPPING OVERFLOW IN UINT256/512 ARITHMETIC + +**Description**: CosmWasm's `Uint256`, `Uint512`, `Int256`, and `Int512` types used wrapping (modular) math for `pow()` and `neg()` instead of panicking on overflow. This means arithmetic silently wraps around on overflow, producing incorrect results. Token swap calculations, LP share pricing, and any exponentiation-based math become manipulable. `Uint128` and `Uint64` are NOT affected (they already used checked math). + +**Affected versions**: `cosmwasm-std` < 1.4.4, < 1.5.4, < 2.0.2 + +**Detection Patterns**: +```rust +// VULNERABLE: Uint256/512 pow used wrapping math (CWA-2024-002) +let price = Uint256::from(base_price).pow(exponent); +// If result overflows 256 bits, it wraps silently instead of panicking +// Token swap calculations yield incorrect amounts +// LP share pricing becomes manipulable + +// VULNERABLE: Int256/512 neg() wraps on MIN value +let negated = Int256::MIN.neg(); +// Wraps to Int256::MIN instead of panicking (two's complement) +``` + +**What to Check**: +- [ ] `cosmwasm-std` version is patched (>= 1.4.4, >= 1.5.4, >= 2.0.2) +- [ ] Any `Uint256::pow()`, `Uint512::pow()`, `Int256::pow()`, `Int512::pow()` usage reviewed for overflow possibility +- [ ] `Int256::neg()` / `Int512::neg()` not called on `MIN` values +- [ ] Contracts using large-number exponentiation for pricing, interest rates, or AMM curves are audited for wrapping behavior on older `cosmwasm-std` + +**Mitigation**: +```rust +// SECURE: Use patched cosmwasm-std (>= 2.0.2) where pow() panics on overflow +// OR: Use checked_pow() if available, or manually validate bounds before exponentiation +let max_safe = Uint256::from(2u128).pow(128); // Know your bounds +assert!(base_price < max_safe, "overflow risk"); +let price = Uint256::from(base_price).pow(exponent); +``` + +**References**: +- [CWA-2024-002](https://github.com/CosmWasm/advisories/blob/main/CWAs/CWA-2024-002.md) + +--- + +### 2 NON-DETERMINISTIC QUERIES FROM COSMWASM CONTRACTS + +**Description**: CosmWasm contracts can call Cosmos SDK queries via Stargate queries. Queries annotated with `cosmos.query.v1.module_query_safe` are callable from within contracts during execution. If such a query is non-deterministic (returns different results on different validators for the same block height) or consumes variable gas, it causes consensus failures. This is especially dangerous because the query runs inside contract execution which is on the consensus path. + +**Detection Patterns**: +```go +// VULNERABLE: Query marked as module_query_safe but produces non-deterministic results +// In proto definition: +// option (cosmos.query.v1.module_query_safe) = true; +// +// But the query handler iterates a map, uses floating point, or returns +// results that vary by node (e.g., local mempool state, peer info) +func (q queryServer) UnsafeQuery(ctx context.Context, req *types.QueryRequest) (*types.QueryResponse, error) { + // Non-deterministic: map iteration order + results := make(map[string]string) + for k, v := range results { // Different order per validator! + // ... + } + return &types.QueryResponse{Data: serialize(results)}, nil +} +``` + +```rust +// CosmWasm contract calling a Stargate query during execution +let response: QueryResponse = deps.querier.query(&QueryRequest::Stargate { + path: "/custom.module.v1.Query/UnsafeQuery".to_string(), + data: request.encode_to_vec().into(), +})?; +// If UnsafeQuery is non-deterministic, this contract execution +// produces different state on different validators → chain halt +``` + +**What to Check**: +- [ ] ALL queries annotated with `cosmos.query.v1.module_query_safe` are deterministic +- [ ] Module-query-safe queries always consume the same gas for the same input (variable gas consumption causes consensus failure) +- [ ] No map iteration, floating point, system time, or other non-deterministic operations in module-query-safe query handlers +- [ ] Stargate query allowlist reviewed — only deterministic queries are permitted + +**References**: +- [CWA-2024-006](https://github.com/CosmWasm/advisories/blob/main/CWAs/CWA-2024-006.md) + +--- + +### 3 IBC REENTRANCY VIA COSMWASM SUBMESSAGES + +**Description**: CosmWasm's submessage mechanism (`SubMsg`) amplifies the IBC reentrancy / CEI violation described in [IBC_VULNERABILITY_PATTERNS.md §16](IBC_VULNERABILITY_PATTERNS.md#16-ibc-reentrancy--cei-violations). When `ibc-hooks` middleware invokes a CosmWasm contract on an IBC callback (e.g., `OnTimeoutPacket`), the contract can re-enter via submessages before the original callback's state changes are finalized. See IBC §16 for the full Go-side vulnerability, affected versions, detection patterns, and checklist. + +This section covers the **CosmWasm-specific attack surface**: how Rust contracts exploit the underlying IBC CEI violation. + +**Vulnerable chain requirements**: ALL of these must be true: +1. IBC-enabled with vulnerable ibc-go version (see [IBC §16](IBC_VULNERABILITY_PATTERNS.md#16-ibc-reentrancy--cei-violations) for versions) +2. CosmWasm-enabled with permissionless contract uploads +3. `ibc-hooks` middleware wrapping ICS-20 transfer + +**CosmWasm Exploit Pattern**: +```rust +// CosmWasm contract exploiting IBC reentrancy via submessages +#[entry_point] +pub fn sudo(deps: DepsMut, env: Env, msg: SudoMsg) -> Result { + match msg { + // Called by ibc-hooks on IBC packet timeout + SudoMsg::IBCLifecycleComplete { .. } => { + // Re-enter by dispatching another timeout via submessage + let msg = CosmosMsg::Stargate { + type_url: "/ibc.core.channel.v1.MsgTimeout".to_string(), + value: craft_timeout_msg(), // Same packet, same sequence + }; + Ok(Response::new().add_submessage(SubMsg::new(msg))) + // Submessage executes before parent's state changes are finalized + // Double-refund: tokens refunded twice for the same timed-out packet + } + } +} +``` + +**CosmWasm-Specific Checks** (in addition to [IBC §16](IBC_VULNERABILITY_PATTERNS.md#16-ibc-reentrancy--cei-violations) checklist): +- [ ] `ibc-hooks` middleware usage reviewed — this is the bridge between IBC callbacks and CosmWasm execution +- [ ] If CosmWasm uploads are permissionless, the attack surface is maximal — consider permissioned uploads or code review requirements + +**Grep Patterns** (use Grep tool with `glob: "*.go"`): +- `ibc-hooks|ibchooks` in `x/` and `app/` — hooks middleware presence +- `InstantiateDefaultPermission|StoreCodeAuthorization` in `x/` and `app/` — upload permissions + +**References**: See [IBC_VULNERABILITY_PATTERNS.md §16](IBC_VULNERABILITY_PATTERNS.md#16-ibc-reentrancy--cei-violations) for full references. + +--- + +### SUMMARY + +| # | Pattern | Impact | Known Incident / Reference | +|---|---------|--------|---------------------------| +| 1 | Wrapping overflow in Uint256/512 | Fund manipulation (pricing, LP shares) | CWA-2024-002 | +| 2 | Non-deterministic queries from CosmWasm | Chain halt (consensus failure) | CWA-2024-006 | +| 3 | IBC reentrancy via CosmWasm submessages | Fund theft ($150M+ at risk, $6.4M stolen) | ASA-2024-007, Terra exploit | + +--- + +### References + +- **CosmWasm Security Advisories**: https://github.com/CosmWasm/advisories +- **ibc-go Security Advisories**: https://github.com/cosmos/ibc-go/security/advisories +- **Asymmetric Research**: "Cosmos IBC Reentrancy Infinite Mint" — https://blog.asymmetric.re/cosmos-ibc-reentrancy-infinite-mint/ diff --git a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/DISCOVERY.md b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/DISCOVERY.md new file mode 100644 index 00000000..5d9734bf --- /dev/null +++ b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/DISCOVERY.md @@ -0,0 +1,67 @@ +## Discovery & CLAUDE.md Generation + +Explore the codebase and write a `CLAUDE.md` at the target repo root with all context needed for the audit. + +### Platform Detection + +Determine what you're auditing: +- **Pure Cosmos SDK**: `go.mod` imports `cosmossdk.io/*` or `github.com/cosmos/cosmos-sdk/*` +- **EVM-based (Ethermint/Evmos)**: `go.mod` imports `github.com/evmos/ethermint` or `github.com/evmos/evmos`; has `x/evm` module +- **CosmWasm**: `go.mod` imports `github.com/CosmWasm/wasmd`; or `.rs` contracts with `use cosmwasm_std::*` +- **IBC enabled**: `go.mod` imports `github.com/cosmos/ibc-go`; has IBC keepers or `x/ibc*` modules + +### Technical Inventory + +Identify and record: +1. SDK version and ibc-go version from `go.mod` +2. Custom modules under `x/*/` — list each with one-line purpose +3. ABCI hooks per module (`HasBeginBlocker`, `HasEndBlocker`, `HasPreBlocker`, `HasPrecommit`, `HasPrepareCheckState`, `HasABCIEndBlock`) +4. Message types from `proto/.../tx.proto` — list each with its `cosmos.msg.v1.signer` field +5. Handlers in `keeper/msg_server.go` — note any stub/no-op implementations +6. Keepers that hold references to `bankKeeper`, `stakingKeeper`, `authzKeeper` +7. AnteHandler chain from `app.go` or `ante.go` — list decorators in order +8. IBC integration: IBC modules, middleware stack, ICA host/controller, PFM + +### Threat Model + +Determine and record: +1. **What the chain does**: DEX, lending, bridge, staking derivatives, NFT, general-purpose, etc. +2. **High-value assets**: tokens held in escrow, module accounts with mint/burn authority, LP pools, vaults +3. **Trust boundaries**: which actors are trusted (validators, governance, relayers, oracles, IBC counterparties) and what each can do +4. **External integrations**: IBC channels (which chains, which tokens), oracles, bridges, off-chain components +5. **Custom crypto or consensus**: vote extensions, custom ABCI++, custom mempool (`PrepareProposal`/`ProcessProposal`), MEV protection +6. **Upgrade history**: recent migrations, deprecated modules still present, parameter changes + +### CLAUDE.md Structure + +Write the CLAUDE.md as a structured reference for the auditor: + +```markdown +# Chain Audit Context + +## Versions +- Cosmos SDK: v0.50.x +- ibc-go: v8.x + +## Modules +| Module | Purpose | ABCI Hooks | Bank Access | +|--------|---------|------------|-------------| +| x/dex | Order book DEX | EndBlocker (match orders) | Yes (escrow) | + +## Message Types +| Message | Signer Field | Handler | +|---------|-------------|---------| +| MsgPlaceOrder | sender | msg_server.go:42 | + +## Threat Model +- **Chain purpose**: DEX with IBC token support +- **High-value targets**: escrow account (holds all deposited tokens), LP pools +- **Trust boundaries**: validators (order matching), relayers (IBC), governance (params) +- **IBC exposure**: channels to Osmosis, Cosmos Hub; PFM enabled + +## AnteHandler Chain +1. SetUpContext → 2. ... → N. SigVerification + +## Audit Focus Areas +[Based on threat model, which vulnerability categories matter most] +``` diff --git a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/EVM_VULNERABILITY_PATTERNS.md b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/EVM_VULNERABILITY_PATTERNS.md new file mode 100644 index 00000000..bb7ba9fa --- /dev/null +++ b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/EVM_VULNERABILITY_PATTERNS.md @@ -0,0 +1,369 @@ +## Cosmos EVM Vulnerability Patterns + +> Vulnerability patterns specific to Cosmos EVM chains (evmOS, Ethermint, Initia, Berachain, Sei, etc.). These chains run two execution runtimes (EVM + Cosmos SDK) with independent state that must stay in sync. This is the most frequently discovered vulnerability class in Cosmos EVM chains — 7+ findings across multiple chains, $7M+ in real losses. Referenced by [SKILL.md](../SKILL.md). + +### 1 EVM REVERT DOES NOT ROLLBACK COSMOS STATE + +**Description**: Precompiles bridge EVM and Cosmos state machines. When a Solidity `revert`, `try/catch`, or out-of-gas (OOG) occurs, EVM state rolls back automatically, but Cosmos keeper writes made during the precompile call persist. An attacker wraps a precompile call in `try/catch`, triggers a revert after the Cosmos-side effect executes, and gets the EVM-side effect undone while keeping the Cosmos-side effect. Every Cosmos EVM chain is vulnerable until it implements snapshot-based atomic precompile wrappers. + +**Detection Patterns**: +```solidity +// VULNERABLE: Solidity try/catch around precompile call +// EVM state rolls back on revert, but Cosmos delegation persists +contract InfiniteDelegate { + IStaking constant STAKING = IStaking(0x...1003); + + function exploit(address validator, uint256 amount) external { + try this._delegate(validator, amount) {} catch {} + // EVM: tokens refunded (revert undid the transfer) + // Cosmos: delegation still exists (keeper write persisted) + // Net effect: delegation increased without balance decreasing + } + + function _delegate(address validator, uint256 amount) external { + STAKING.delegate(validator, amount); + revert(); // EVM state rolls back, Cosmos state does NOT + } +} +``` + +**What to Check**: +- [ ] ALL precompile `Run()` methods wrapped in atomic snapshot/revert (no partial execution on OOG or error) +- [ ] EVM `revert` / `try/catch` / OOG propagates rollback to Cosmos keeper state — test with Solidity `try/catch` around every precompile call +- [ ] IBC operations triggered via precompiles are atomic with the EVM transaction + +**References**: +- [EVM staking precompile infinite delegation](https://forum.cosmos.network/t/critical-evm-precompiled-contract-bug-allowing-unlimited-token-mint/14060) (Finding 7) +- [Saga $7M exploit](https://advisories.gitlab.com/pkg/golang/github.com/cosmos/evm/GHSA-54gx-3cgr-7mfm/) (Finding 5) + +--- + +### 2 STATEDB.COMMIT() DURING PRECOMPILE EXECUTION + +**Description**: Certain precompile calls (e.g., `delegationTotalRewards`) trigger `StateDB.Commit()`, writing state to permanent storage mid-execution. A subsequent `revert` only undoes cached in-memory state — permanent storage retains the committed state. This creates orphaned contracts with withdrawable balances. Exponential multiplication is possible: 1 ETH becomes 1,048,576 ETH in 20 loops. + +**Detection Patterns**: +```go +// VULNERABLE: Precompile triggers StateDB.Commit() (Finding 6) +// Calling certain precompiles caused StateDB.Commit() to write +// state to permanent storage. A subsequent revert only undoes +// cached in-memory state, but permanent storage retains the +// committed state. +// Result: Orphaned contracts with balances are accessible and withdrawable. +``` + +**What to Check**: +- [ ] `StateDB.Commit()` is NEVER called during precompile execution (only at transaction finalization) +- [ ] Precompile code paths audited for any indirect `Commit()` triggers (e.g., via keeper calls that flush caches) +- [ ] StateDB journal entries cover all state modifications made by precompiles + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `StateDB|stateDB` — then check results for `commit` context +- `func.*Run\(.*vm\.EVM` — precompile Run methods + +**References**: +- [Evmos infinite mint](https://blog.asymmetric.re/evmos-precompile-state-commit-infinite-mint/) (Finding 6) + +--- + +### 3 PARTIAL PRECOMPILE EXECUTION (OOG) + +**Description**: A precompile `Run()` performs multiple Cosmos state changes sequentially (e.g., transfer rewards then clear claimable counter). If execution runs out of gas between steps, early state changes persist while later ones are skipped. The attacker receives rewards but the claimable counter is never reset — repeat to drain the module. + +**Detection Patterns**: +```go +// VULNERABLE: Precompile Run() not atomic (Finding 20) +func (p DistributionPrecompile) Run(evm *vm.EVM, input []byte) ([]byte, error) { + // Step 1: Transfer rewards to user (succeeds) + k.bankKeeper.SendCoins(ctx, moduleAcc, userAddr, rewards) + // Step 2: Clear claimable rewards (runs out of gas here!) + k.SetClaimableRewards(ctx, userAddr, sdk.Coins{}) + // User received rewards but claimable counter never reset + // Repeat to drain distribution module +} +``` + +**What to Check**: +- [ ] Every precompile `Run()` is wrapped in an atomic execution wrapper (snapshot before, revert on any error including OOG) +- [ ] No multi-step state mutations without atomicity guarantees +- [ ] Gas estimation for precompile operations is accurate (underestimation enables OOG at attacker-chosen points) + +**Mitigation**: +```go +// SECURE: Wrap in atomic execution +func (p DistributionPrecompile) Run(evm *vm.EVM, input []byte) ([]byte, error) { + return p.RunAtomic(evm, input, func(ctx sdk.Context) ([]byte, error) { + // All operations atomic — snapshot + revert on any error including OOG + }) +} +``` + +**References**: +- [Partial precompile state writes](https://github.com/advisories/GHSA-mjfq-3qr2-6g84) (Finding 20) + +--- + +### 4 REVERTED EVM CALLS LEAVE COSMOS MESSAGES IN QUEUE + +**Description**: When an EVM call dispatches a Cosmos message (e.g., IBC transfer) via a precompile, and the EVM call subsequently reverts, the dispatched Cosmos message remains in the execution queue. The Cosmos message executes despite the EVM revert, enabling IBC transfers (and other Cosmos-side effects) without corresponding EVM-side state changes. + +**Detection Patterns**: +```go +// VULNERABLE: Initia — dispatched Cosmos messages remain in execution queue +// even when the EVM call that dispatched them reverts (Finding 26b) +// Solidity try/catch wraps a call that dispatches IBC transfer via precompile +// Revert undoes EVM state but IBC transfer message still in Cosmos execution queue +// Result: IBC transfers execute despite explicit reversion +``` + +**What to Check**: +- [ ] Cosmos message dispatch queue is cleared when EVM execution reverts +- [ ] All Cosmos messages dispatched from precompiles are tracked in the EVM journal and rolled back on revert +- [ ] Test: wrap every message-dispatching precompile call in Solidity `try/catch` with explicit `revert` — verify no Cosmos messages execute + +**References**: +- [Initia Code4rena report](https://code4rena.com/reports/2025-02-initia-cosmos) (Finding 26b) + +--- + +### 5 GAS LIMIT BYPASS VIA PRECOMPILE DISPATCH + +**Description**: Solidity's `call{gas: X}()` restricts gas forwarded to an untrusted contract. But if that contract calls a precompile that dispatches a new Cosmos `MsgCall`, the new message gets the full parent transaction gas limit — not the restricted amount. This defeats gas-based reentrancy guards and any logic relying on gas limits to bound untrusted execution. + +**Detection Patterns**: +```solidity +// VULNERABLE: Initia — COSMOS_CONTRACT precompile bypasses gas restrictions (Finding 26c) +// Solidity call{gas: X}(untrustedContract) restricts gas +// But untrusted contract calls COSMOS_CONTRACT precompile to dispatch new MsgCall +// New MsgCall gets full parent transaction gas limit, not the restricted amount +``` + +**What to Check**: +- [ ] Gas forwarding through precompile dispatch does not bypass caller-imposed gas limits +- [ ] Precompile-dispatched messages inherit the remaining gas of the calling frame, not the transaction-level gas limit +- [ ] Gas-based reentrancy guards are not the sole defense (combine with mutex/state guards) + +**References**: +- [Initia Code4rena report](https://code4rena.com/reports/2025-02-initia-cosmos) (Finding 26c) + +--- + +### 6 WRONG ROUTING IN MULTI-DENOM ERC20 OPERATIONS + +**Description**: When processing multiple denominations in a loop, ERC20-related operations (burn, mint, community pool funding) accidentally use the full `amount` parameter (all denoms) instead of the individual `coin` being processed. If the loop contains both ERC20 and native denoms, the native denom amount is sent to the wrong destination on every iteration that hits the ERC20 path. + +**Detection Patterns**: +```go +// VULNERABLE: Initia — BurnCoins sends entire amount to community pool (Finding 26a) +// When burning ERC20 denominated coins in a loop, +// the function sends the full `amount` parameter (all denoms) +// instead of just the individual `coin` being processed +func (k Keeper) BurnCoins(ctx sdk.Context, amount sdk.Coins) error { + for _, coin := range amount { + if k.isERC20(coin.Denom) { + // WRONG: sends ALL coins, not just this one + k.communityPoolKeeper.FundCommunityPool(ctx, amount, moduleAddr) + } + } +} +``` + +**What to Check**: +- [ ] Multi-denom operations in loops use the individual `coin`, not the full `amount` parameter +- [ ] ERC20/native denom branching logic tested with mixed-denom inputs +- [ ] Community pool / burn / mint amounts match the specific coin being processed + +**Mitigation**: +```go +// SECURE: Use the individual coin in the loop +func (k Keeper) BurnCoins(ctx sdk.Context, amount sdk.Coins) error { + for _, coin := range amount { + if k.isERC20(coin.Denom) { + k.communityPoolKeeper.FundCommunityPool(ctx, sdk.NewCoins(coin), moduleAddr) + } else { + k.bankKeeper.BurnCoins(ctx, moduleName, sdk.NewCoins(coin)) + } + } +} +``` + +**References**: +- [Initia Code4rena report](https://code4rena.com/reports/2025-02-initia-cosmos) (Finding 26a) + +--- + +### 7 ETHERMINT ANTE HANDLER BYPASS (NESTED MESSAGES) + +**Description**: Cosmos EVM chains add custom AnteHandler decorators for EVM-specific validation (gas price floors, tx type restrictions, EVM-specific fee checks). These decorators typically only inspect the outer message. An attacker wraps a blocked EVM message inside `x/authz MsgExec` or an `x/group` proposal — the inner message skips EVM-specific ante validation entirely. + +**Detection Patterns**: +```go +// VULNERABLE: AnteHandler only checks outer message type +func (d EthGasDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + for _, msg := range tx.GetMsgs() { + // Only checks outer msg — does NOT recurse into MsgExec.Msgs! + if ethMsg, ok := msg.(*evmtypes.MsgEthereumTx); ok { + if err := d.validateGas(ethMsg); err != nil { + return ctx, err + } + } + } + return next(ctx, tx, simulate) +} +``` + +**What to Check**: +- [ ] Custom EVM AnteHandlers recurse into `x/authz MsgExec` and `x/group` proposal inner messages +- [ ] EVM-specific fee/gas validation applies to nested EVM messages, not just top-level +- [ ] SDK `MaxUnpackAnyRecursionDepth = 10` is not sufficient — semantic nesting (authz wrapping authz) must be handled by the ante handler itself +- [ ] Integration tests submit EVM messages wrapped in `MsgExec` to verify they are validated + +**Mitigation**: +```go +// SECURE: Recurse into nested messages +func (d EthGasDecorator) checkMsg(msg sdk.Msg) error { + if ethMsg, ok := msg.(*evmtypes.MsgEthereumTx); ok { + return d.validateGas(ethMsg) + } + if exec, ok := msg.(*authz.MsgExec); ok { + for _, inner := range exec.Msgs { + var innerMsg sdk.Msg + if err := d.cdc.UnpackAny(inner, &innerMsg); err != nil { + return err + } + if err := d.checkMsg(innerMsg); err != nil { + return err + } + } + } + return nil +} +``` + +**References**: +- [Jump Crypto — Bypassing Ethermint Ante Handlers](https://jumpcrypto.com/bypassing-ethermint-ante-handlers/) + +--- + +### 8 EVM HOOKS AND CROSS-MODULE INTERACTIONS + +**Description**: Cosmos EVM chains use hooks (`PostTxProcessing`, `PreTxProcessing`) to synchronize EVM and Cosmos state after EVM transactions. The `x/evm` and `x/erc20` modules fire these hooks on every EVM transaction. Bugs in hook logic — missing error propagation, state assumptions, reentrancy via hook callbacks — break the cross-module state invariant. + +**What to Check**: +- [ ] [EVM hooks](https://docs.evmos.org/protocol/modules/evm#hooks) from `x/evm` and `x/erc20` modules reviewed for correctness +- [ ] Hook errors propagate correctly (a failed hook must revert the entire EVM transaction) +- [ ] Hooks do not assume EVM state is finalized (it may still be in a cached context) +- [ ] Cross-module interactions between `x/evm`, `x/erc20`, `x/bank`, and `x/staking` do not create circular dependencies or reentrancy +- [ ] User interaction with [module special addresses](https://docs.evmos.org/protocol/module-accounts) reviewed — EVM contracts should not be able to interact with Cosmos module accounts in unexpected ways + +--- + +### 9 ETHEREUM ADDRESS VALIDATION + +**Description**: Cosmos EVM chains handle two address formats: Bech32 (`cosmos1...`) and hex (`0x...`). The `go-ethereum` function `common.HexToAddress()` does NOT validate format and silently ignores errors — it returns a zero-padded address for any invalid input. Address mapping between the two formats must be bijective and validated. + +**Detection Patterns**: +```go +// VULNERABLE: HexToAddress does not verify format, ignores errors +addr := common.HexToAddress(userInput) +// "not_an_address" → 0x0000000000000000000000000000000000000000 +// No error returned! Silently maps to zero address. + +// VULNERABLE: Inconsistent address derivation +cosmosAddr := sdk.AccAddress(ethAddr.Bytes()) // Cosmos addr from ETH addr +ethAddr2 := common.BytesToAddress(cosmosAddr) // Round-trip may not match +``` + +**What to Check**: +- [ ] NOT using `common.HexToAddress()` for user-supplied input — use a validating function instead +- [ ] Address conversion between Bech32 and hex is validated in both directions +- [ ] Zero address (`0x000...`) is explicitly rejected where it should not be valid +- [ ] EVM contract addresses derived from Cosmos module accounts are deterministic and documented + +**Mitigation**: +```go +// SECURE: Validate hex address format before conversion +func ValidateHexAddress(addr string) (common.Address, error) { + if !common.IsHexAddress(addr) { + return common.Address{}, fmt.Errorf("invalid hex address: %s", addr) + } + return common.HexToAddress(addr), nil +} +``` + +--- + +### 10 CROSS-CHAIN TRANSACTION REPLAY AND CHAIN ID + +**Description**: Cosmos EVM chains use an epoch-based chain ID format (`chainname_NNNN-1`) where the numeric part maps to the Ethereum chain ID used in EVM transaction signing. If the chain ID epoch is not correctly parsed or enforced, transactions signed for one chain can be replayed on another. Nonce management differs between EVM (per-account incrementing) and Cosmos (sequence numbers) — mismatches enable replay or stuck transactions. + +**What to Check**: +- [ ] Chain ID epoch is correctly parsed and used as the EVM chain ID in `ethermint/types/chain_id.go` +- [ ] EVM transactions are validated against the current chain ID (not just any valid chain ID) +- [ ] Nonce management is synchronized between EVM and Cosmos account sequences +- [ ] Chain upgrades that change the chain ID epoch correctly invalidate pending EVM transactions +- [ ] `eth_chainId` RPC returns the correct value matching the Cosmos chain ID epoch + +--- + +### SUMMARY: COSMOS EVM BUG CLASSES + +| # | Pattern | Impact | Known Incident / Reference | +|---|---------|--------|---------------------------| +| 1 | EVM revert doesn't rollback Cosmos state | Fund theft (infinite delegation/mint) | Saga $7M, Finding 5/7 | +| 2 | StateDB.Commit() during precompile | Infinite mint (exponential) | Evmos, Finding 6 | +| 3 | Partial precompile execution (OOG) | Module drainage | GHSA-mjfq-3qr2-6g84, Finding 20 | +| 4 | Reverted EVM calls leave Cosmos msgs | Unauthorized IBC transfers | Initia, Finding 26b | +| 5 | Gas limit bypass via precompile dispatch | Reentrancy guard defeat | Initia, Finding 26c | +| 6 | Wrong routing in multi-denom ERC20 ops | Fund misrouting | Initia, Finding 26a | +| 7 | Ethermint ante handler bypass | Validation skip via nested msgs | Jump Crypto disclosure | +| 8 | EVM hooks cross-module interactions | State desync | Multiple chains | +| 9 | Ethereum address validation | Zero address injection | go-ethereum HexToAddress | +| 10 | Cross-chain tx replay / chain ID | Transaction replay | Chain ID epoch parsing | + +--- + +### GREP PATTERNS (ALL PATTERNS) + +Use the Grep tool with `glob: "*.go"` in `x/` for all searches below. + +**Precompile implementations**: +- `func.*Run\(.*vm\.EVM` — precompile Run methods +- `precompile|Precompile` — precompile files + +**StateDB usage**: +- `StateDB|stateDB` — then check results for `commit` context +- `stateDB\.(Snapshot|RevertToSnapshot)` — snapshot/revert usage + +**EVM hooks**: +- `PostTxProcessing|PreTxProcessing|EvmHooks` + +**ERC20 module**: +- `isERC20|IsERC20|erc20` — ERC20 detection +- `RegisterERC20|RegisterCoin|ToggleConversion` — token conversion + +**Address handling**: +- `HexToAddress|IsHexAddress|BytesToAddress` + +**Ante handlers**: +- `AnteHandle|AnteDecorator` — handler files +- `MsgEthereumTx|EthereumTx` — EVM message type + +**Chain ID**: +- `ChainID|ParseChainID|EpochNumber` + +**Solidity patterns** (use `glob: "*.sol"` in `contracts/`): +- `try|catch|revert` — revert/try-catch patterns + +--- + +### References + +- **Saga $7M exploit**: https://advisories.gitlab.com/pkg/golang/github.com/cosmos/evm/GHSA-54gx-3cgr-7mfm/ +- **Evmos infinite mint**: https://blog.asymmetric.re/evmos-precompile-state-commit-infinite-mint/ +- **EVM staking precompile**: https://forum.cosmos.network/t/critical-evm-precompiled-contract-bug-allowing-unlimited-token-mint/14060 +- **Partial precompile state writes**: https://github.com/advisories/GHSA-mjfq-3qr2-6g84 +- **Initia Code4rena**: https://code4rena.com/reports/2025-02-initia-cosmos +- **Ethermint ante handler bypass**: https://jumpcrypto.com/bypassing-ethermint-ante-handlers/ +- **Evmos module accounts**: https://docs.evmos.org/protocol/module-accounts +- **Evmos EVM hooks**: https://docs.evmos.org/protocol/modules/evm#hooks diff --git a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/IBC_VULNERABILITY_PATTERNS.md b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/IBC_VULNERABILITY_PATTERNS.md new file mode 100644 index 00000000..a836bad0 --- /dev/null +++ b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/IBC_VULNERABILITY_PATTERNS.md @@ -0,0 +1,493 @@ +## IBC Vulnerability Patterns & Audit Checks + +> IBC-specific vulnerabilities for Cosmos chains integrating ibc-go. 16 vulnerability patterns covering middleware bugs, channel/denom handling, Interchain Accounts misuse, reentrancy, and general IBC integration checks. Referenced by [SKILL.md](../SKILL.md). + +### 1 UNTRUSTED IBC COUNTERPARTY CHAIN + +**Description**: An attacker can create a malicious chain and [spoof IBC message fields](https://github.com/evmos/evmos/security/advisories/GHSA-5jgq-x857-p8xw). The counterparty chain controls the `sender`, `memo`, and other packet data fields. Any IBC middleware or hook that acts on these fields must validate against a channel/counterparty whitelist. With ICS-20 v2 forwarding (ibc-go v9+), a forwarded packet may originate from a chain several hops away with no direct trust relationship. + +**What to Check**: +- [ ] Chains that can connect are authorized via a channel/counterparty whitelist +- [ ] IBC middleware and hooks that act on `sender`, `memo`, or other packet data fields validate the source channel +- [ ] With ICS-20 v2 forwarding, multi-hop trust implications are considered (forwarded packets may originate from untrusted chains) + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `OnRecvPacket|OnAcknowledgementPacket` — then check results for `sender` or `memo` usage + +**References**: +- [GHSA-5jgq-x857-p8xw](https://github.com/evmos/evmos/security/advisories/GHSA-5jgq-x857-p8xw) + +--- + +### 2 UNSAFE LookupModuleByPort AND LookupModuleByChannel USAGE + +**Description**: **[ibc-go < v10 only]** [`LookupModuleByPort` and `LookupModuleByChannel`](https://github.com/cosmos/ibc-go/issues/71) were problematic APIs in the IBC capability module. They were removed in v10 along with the capability module ([#7239](https://github.com/cosmos/ibc-go/pull/7239), [#7252](https://github.com/cosmos/ibc-go/pull/7252)), replaced by a static port router (`PortKeeper.GetRoute()`). On older ibc-go versions, incorrect usage of these APIs can lead to wrong module routing. + +**What to Check**: +- [ ] If module has access to `ibc` keeper, `LookupModuleByPort` and `LookupModuleByChannel` usages are carefully reviewed +- [ ] On ibc-go v10+, verify these APIs are no longer used and the static port router is used instead + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `LookupModuleByPort|LookupModuleByChannel` + +**References**: +- [ibc-go#71](https://github.com/cosmos/ibc-go/issues/71) +- [#7239](https://github.com/cosmos/ibc-go/pull/7239) +- [#7252](https://github.com/cosmos/ibc-go/pull/7252) + +--- + +### 3 MISSING IBC CHANNEL CLOSE VALIDATION + +**Description**: If a user should NOT be able to close an IBC channel, the `OnChanCloseInit` callback must return an error. Failing to do so allows any user to close the channel, disrupting the IBC application. The transfer module [explicitly rejects channel close](https://github.com/cosmos/ibc-go/blob/v8.5.2/modules/apps/transfer/ibc_module.go#L159-L166) as a reference pattern. + +**Detection Patterns**: +```go +// VULNERABLE: Missing or permissive channel close callback +func (im IBCModule) OnChanCloseInit(ctx sdk.Context, portID, channelID string) error { + return nil // Allows anyone to close the channel! +} +``` + +**What to Check**: +- [ ] `OnChanCloseInit` returns an error if channels should not be closable by users +- [ ] Channel close behavior is explicitly tested + +**Mitigation**: +```go +// SECURE: Reject channel close +func (im IBCModule) OnChanCloseInit(ctx sdk.Context, portID, channelID string) error { + return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "channel close not allowed") +} +``` + +--- + +### 4 INCORRECT IBC PACKET TIMEOUT HANDLING + +**Description**: When an IBC packet times out or fails, the application must correctly refund tokens. On the source chain, escrowed tokens must be unescrowed; on the sink chain, burned vouchers must be minted back. See [`refundPacketToken`](https://github.com/cosmos/ibc-go/blob/v8.5.2/modules/apps/transfer/keeper/relay.go) in v8 (renamed to `refundPacketTokens` in v10 with `InternalTransferRepresentation` type). The core logic (source chain = unescrow, sink chain = mint back) is unchanged across versions. + +**What to Check**: +- [ ] Application handles packet timeout correctly — tokens are refunded on failed sends +- [ ] Refund logic uses the same source/sink determination as the send path +- [ ] `OnTimeoutPacket` is implemented and tested for all packet types +- [ ] State changes from a failed send are fully rolled back + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `OnTimeoutPacket|refundPacketToken` + +--- + +### 5 INCORRECT IBC CHANNEL ORDERING TYPE + +**Description**: ORDERED channels are [closed on timeout](https://github.com/cosmos/ibc-go/blob/v8.5.2/modules/core/04-channel/keeper/timeout.go) — if a single packet times out, the entire channel is permanently closed. If message order doesn't matter, use UNORDERED to avoid this. Note: `ORDERED_ALLOW_TIMEOUT` was proposed but never merged into ibc-go. + +**What to Check**: +- [ ] Channel type (ORDERED vs UNORDERED) matches application invariants +- [ ] If using ORDERED channels, the application handles channel closure on timeout gracefully +- [ ] If message order doesn't matter, UNORDERED is used to avoid timeout-triggered channel closure + +--- + +### 6 IBC TRANSFERS BREAKING INTERNAL BOOKKEEPING + +**Description**: IBC transfers always mint/burn coins via `x/bank`. If a module maintains internal accounting (e.g., total deposited, collateral tracking), IBC transfers can silently break those invariants by moving tokens without going through the module's accounting path. Use blocklist, `SendEnabled`, or `SendRestriction` to control. In newer Cosmos SDK versions, `SendRestriction` is checked before deducting coins (SDK [#20517](https://github.com/cosmos/cosmos-sdk/pull/20517)), making it a more reliable defense. With ICS-20 v2 forwarding, tokens can arrive via unexpected multi-hop paths, amplifying bookkeeping risks. + +**What to Check**: +- [ ] IBC transfers cannot bypass internal bookkeeping for tokens managed by the module +- [ ] Module uses blocklist, `SendEnabled`, or `SendRestriction` to prevent unauthorized IBC movement of managed tokens +- [ ] With ICS-20 v2 forwarding, multi-hop token arrival paths are considered + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `SendRestriction|BlockedAddr|SendEnabled` + +**References**: +- [SDK #20517](https://github.com/cosmos/cosmos-sdk/pull/20517) + +--- + +### 7 NON-DETERMINISTIC JSON IN IBC ACKNOWLEDGEMENTS + +**Description**: Custom IBC middleware that constructs or deserializes acknowledgement bytes using Go's `encoding/json` can produce non-deterministic results across different validators. Go's JSON unmarshaller does not guarantee consistent output for map key ordering, numeric precision for large numbers, unknown field handling, and Unicode normalization. If any middleware in the chain's IBC application stack introduces a custom ack format or manipulates ack bytes using standard JSON, different validators may compute different state transitions, halting the chain. Any user that can open an IBC channel can introduce this state — this is trivially exploitable. + +**Known incidents**: ISA-2025-001 (GHSA-4wf3-5qj9-368v), ASA-2025-004 (GHSA-jg6f-48ff-5xrw) +**Affected versions**: ibc-go >= v7 (earlier versions may also be affected) +**Patched in**: v7.10.0, v8.7.0 + +**Detection Patterns**: +```go +// VULNERABLE: Using encoding/json for IBC acknowledgements +func (m MyMiddleware) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, ack []byte) error { + var result map[string]interface{} + json.Unmarshal(ack, &result) // NON-DETERMINISTIC for maps! + newAck, _ := json.Marshal(result) // Different output across validators + // ... +} +``` + +**What to Check**: +- [ ] Custom IBC middleware does NOT produce acknowledgement bytes using `json.Marshal` on types containing maps or floating-point numbers +- [ ] Acknowledgement serialization uses protobuf (not JSON), or a deterministic JSON marshaller (e.g., `cosmossdk.io/x/tx/signing/aminojson`) +- [ ] All middleware in the IBC stack uses deterministic serialization +- [ ] Tests do not assume determinism from running on a single machine + +**Mitigation**: +```go +// SECURE: Use protobuf for acknowledgement serialization +func (m MyMiddleware) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) ibcexported.Acknowledgement { + // Use the channeltypes ack types which serialize via protobuf + return channeltypes.NewResultAcknowledgement([]byte{byte(1)}) +} +``` + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `json\.(Marshal|Unmarshal)` — then check results for `ack` or `acknowledgement` context + +**References**: +- [ISA-2025-001](https://github.com/cosmos/ibc-go/security/advisories/GHSA-4wf3-5qj9-368v) +- [ASA-2025-004](https://github.com/cosmos/ibc-go/security/advisories/GHSA-jg6f-48ff-5xrw) + +--- + +### 8 PFM INVERTED ESCROW/MINT LOGIC + +**Description**: In `packet-forward-middleware`, the `moveFundsToUserRecoverableAccount()` function (for nonrefundable forwarded packets) has the `if/else` branches for escrow vs. mint **inverted** relative to what `SendTransfer()` actually does. When a nonrefundable forward fails, the buggy code MINTS new tokens instead of UNESCROWING (or vice versa), inflating supply. An attacker sends tokens from Chain A to Chain B with a PFM forward to Chain C (marked nonrefundable). When the forward fails, PFM mints new tokens instead of unescrowing. The attacker sends the minted vouchers back to Chain A, draining A's escrow of real tokens. The attack is repeatable: each round inflates supply by the transfer amount. + +**Detection Patterns**: +``` +In relay.go SendTransfer(): + if HasPrefix(sourcePort, sourceChannel) -> BURN + else (!HasPrefix) -> ESCROW + +In WriteAcknowledgementForForwardedPacket() [CORRECT, non-nonrefundable path]: + if !HasPrefix -> tokens were ESCROWED -> code UNESCROWS from escrow [correct] + else HasPrefix -> tokens were BURNED -> code MINTS back [correct] + +In moveFundsToUserRecoverableAccount() [BUGGY, nonrefundable path]: + if !HasPrefix -> tokens were ESCROWED -> code MINTS (WRONG!) + else HasPrefix -> tokens were BURNED -> code UNESCROWS (WRONG!) +``` + +**What to Check**: +- [ ] Every code path that reverses a transfer uses the same source/sink logic (`HasPrefix`) as the original send +- [ ] Nonrefundable forwarding paths have explicit tests comparing escrow/mint direction against the refundable path +- [ ] PFM is fuzz-tested with various denom trace configurations + +**Mitigation**: +```go +// SECURE: Verify source/sink logic is consistent across all refund paths +// The escrow/mint decision in error-handling paths must mirror SendTransfer(): +// !HasPrefix (escrowed on send) -> UNESCROW on refund +// HasPrefix (burned on send) -> MINT on refund +``` + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `HasPrefix|SenderChainIsSource|ReceiverChainIsSource` — source/sink logic +- `nonrefundable|Nonrefundable` — nonrefundable forwarding + +--- + +### 9 ICS-20 V2 MULTI-TOKEN PARTIAL RECEIVE + +**Description**: ICS-20 v2 allows sending multiple tokens in a single packet. The `onRecvPacket` function processes them sequentially in a `for` loop. If token N fails (e.g., insufficient escrow for unescrow), the loop breaks and returns an error acknowledgement. However, tokens 1..N-1 have already been minted/unescrowed on the receiver chain and are **NOT rolled back**. The sender chain then receives the error ack and calls `refundTokens`, which refunds **ALL** tokens — including the ones that were successfully delivered to the receiver. Tokens end up existing on both chains simultaneously, created from nothing. The ICS-20 v2 spec pseudocode itself contains this bug — implementers who faithfully translate the spec inherit it. + +**Detection Patterns**: +```go +// VULNERABLE: Non-atomic multi-token processing +func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) error { + var data types.FungibleTokenPacketDataV2 + // ... + for _, token := range data.Tokens { + // Each token modifies state immediately! + if err := k.processToken(ctx, token); err != nil { + return err // Tokens 1..N-1 already committed, no rollback + } + } + return nil +} +``` + +**What to Check**: +- [ ] Multi-token `onRecvPacket` is wrapped in `CacheContext` to make all operations atomic +- [ ] State is only committed if ALL tokens process successfully +- [ ] Tests cover packets where the Nth token fails for all values of N (not just happy path or first-token-fails) +- [ ] The ICS-20 v2 spec pseudocode itself contains this bug — verify the implementation does NOT faithfully copy it + +**Mitigation**: +```go +// SECURE: Wrap multi-token processing in CacheContext +func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) error { + var data types.FungibleTokenPacketDataV2 + // ... + cacheCtx, writeFn := ctx.CacheContext() + for _, token := range data.Tokens { + if err := k.processToken(cacheCtx, token); err != nil { + return err // Discard all cached writes + } + } + writeFn() // Commit only if ALL tokens succeeded + return nil +} +``` + +--- + +### 10 UNPERMISSIONED IBC CHANNEL OPENING + +**Description**: By default, anyone can open an IBC channel to any chain. If a chain's IBC application doesn't properly validate channel parameters (version, ordering, counterparty), a malicious actor can open channels with unexpected configurations. This was the precondition for the non-deterministic JSON ack vulnerability (§7) — "any user that can open an IBC channel can introduce this state." + +**Detection Patterns**: +```go +// VULNERABLE: No validation in channel open callbacks +func (im IBCModule) OnChanOpenInit(ctx sdk.Context, order channeltypes.Order, + connectionHops []string, portID string, channelID string, + chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, + version string) (string, error) { + return version, nil // Accepts ANY version, ordering, counterparty! +} +``` + +**What to Check**: +- [ ] `OnChanOpenInit` / `OnChanOpenTry` implement strict version and ordering checks +- [ ] Channel opening is permissioned (governance-gated) for sensitive applications +- [ ] IBC application version strings are validated against an allowlist +- [ ] Data from the counterparty chain is never trusted without validation +- [ ] Unexpected port bindings are rejected + +**Mitigation**: +```go +// SECURE: Strict validation in channel open +func (im IBCModule) OnChanOpenInit(ctx sdk.Context, order channeltypes.Order, + connectionHops []string, portID string, channelID string, + chanCap *capabilitytypes.Capability, counterparty channeltypes.Counterparty, + version string) (string, error) { + if order != channeltypes.UNORDERED { + return "", errorsmod.Wrapf(channeltypes.ErrInvalidChannelOrdering, + "expected %s, got %s", channeltypes.UNORDERED, order) + } + if version != types.Version { + return "", errorsmod.Wrapf(types.ErrInvalidVersion, + "expected %s, got %s", types.Version, version) + } + return version, nil +} +``` + +**Grep Patterns** (use Grep tool in `x/`): +- `OnChanOpenInit|OnChanOpenTry` + +--- + +### 11 IBC DENOM / VOUCHER CONFUSION + +**Description**: IBC denomination traces encode the full path a token has traveled. The `ibc/HASH` format hides the actual trace, and developers often fail to validate that a received denom is legitimate. Key concepts that are frequently confused: **Source zone** (token native to sending chain — escrowed on sender, minted as voucher on receiver), **Sink zone** (token returning to origin — voucher burned on sender, unescrowed on receiver), **Trace path** (`transfer/channel-0/transfer/channel-1/uatom` — encodes every hop), **Hashed denom** (`ibc/27394FB092D2ECCD56...` — SHA256 hash of the trace path). In ICS-20 v2, `MsgSendPacket` allowed users to specify `IBCDenom()` (the hashed form) instead of requiring the full trace, letting users bypass trace validation (cosmos/ibc-go#7848). Before ibc-go v9, `FungibleTokenPacketData` JSON unmarshalling silently accepted unknown fields, allowing packet manipulation (cosmos/ibc-go#6215). + +**Known issue**: cosmos/ibc-go#7848 + +**What to Check**: +- [ ] Full denom trace is validated, not just the base denomination +- [ ] Denom whitelists are implemented for sensitive operations (collateral, governance) +- [ ] Security model of each IBC path is documented (which chains are trusted) +- [ ] Unknown fields in packet data unmarshalling are rejected +- [ ] DEXes/lending protocols do NOT treat `ibc/HASH_A` and `ibc/HASH_B` (both with base `uatom`) as equivalent +- [ ] `uatom` arriving via `channel-0` and `uatom` arriving via `channel-1` are treated as DIFFERENT tokens (different security properties) + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `IBCDenom|DenomTrace|GetFullDenomPath` — denom trace handling +- `ibc/` — then check results for `denom`, `token`, or `transfer` context + +--- + +### 12 MISSING IBC RATE LIMITING + +**Description**: Without rate limiting, a single exploit can drain an entire escrow in one block. Osmosis's rate limiting was explicitly credited with slowing down potential damage from the $126M reentrancy bug (ASA-2024-007). + +**What to Check**: +- [ ] Per-channel, per-denom, or aggregate rate limits are implemented on IBC transfers +- [ ] Both inflows AND outflows are rate limited +- [ ] Rate limits are set based on percentage of total supply, not absolute amounts +- [ ] Channel opening itself is rate limited +- [ ] Rate limit hits are monitored as an early warning system +- [ ] Rate limits are governance-adjustable + +**Grep Patterns** (use Grep tool in `x/`): +- `RateLimit|rate_limit|ratelimit` + +**References**: +- [Osmosis rate limits](https://www.range.org/blog/ibc-rate-limits-introduction-and-state-of-the-art-1-3) +- [Cosmos Hub proposal](https://forum.cosmos.network/t/proposal-890-passed-signaling-proposal-ibc-rate-limiting/13410) + +--- + +### 13 ICA HOST ALLOW-ALL CONFIGURATION + +**Description**: The Interchain Accounts (ICA) host module, when enabled, accepts a parameter `allow_messages` that controls which message types can be executed by remote controller chains. The default genesis configuration sets this to `["*"]` — allowing ALL message types. If a chain enables ICA host with `"*"` and any other chain has a controller account on it, that controller chain can execute *any* message on the host chain, including governance proposals, staking operations, and module-specific messages. A chain may have ICA host enabled in its genesis JSON without the host keeper actually being wired, giving a false sense of security that disappears if the keeper is later connected (cosmos/ibc-go#6324). + +**Detection Patterns**: +```json +// VULNERABLE: Default genesis allows all messages +"host_genesis_state": { + "params": { + "host_enabled": true, + "allow_messages": ["*"] + } +} +``` + +**What to Check**: +- [ ] `allow_messages` is set to a minimal whitelist — NEVER `"*"` in production +- [ ] Controller chains that have accounts on the host are audited +- [ ] ICA host enablement is governance-gated +- [ ] A chain does not have ICA host enabled in genesis JSON without the host keeper actually being wired (false sense of security — cosmos/ibc-go#6324) + +**Mitigation**: +```json +// SECURE: Minimal whitelist +"host_genesis_state": { + "params": { + "host_enabled": true, + "allow_messages": [ + "/cosmos.bank.v1beta1.MsgSend", + "/cosmos.staking.v1beta1.MsgDelegate" + ] + } +} +``` + +**Grep Patterns** (use Grep tool in `x/`): +- `allow_messages|AllowMessages` with `glob: "*.{go,json}"` — ICA allow list +- `RegisterInterchainAccount|SendTx` — ICA usage + +**References**: +- [Architecture review](https://github.com/cosmos/ibc-go/issues/631) +- [Genesis default issue](https://github.com/cosmos/ibc-go/issues/6324) + +--- + +### 14 ICA CHANNEL ORDERING BACKWARDS COMPATIBILITY + +**Description**: In ibc-go v7.5.0+, the default channel ordering for `MsgRegisterInterchainAccount` changed from `ORDER_ORDERED` to `ORDER_UNORDERED`. Older ibc-go versions only support `ORDER_ORDERED` for ICA channels. If a chain upgrades to ibc-go >= 7.5.0 and tries to open an ICA channel to a chain running an older version, the channel open will fail silently or explicitly. Additionally, ICA channel upgrades that change the ordering (ordered to unordered or vice versa) are explicitly disallowed by a guard that had to be added after the fact (cosmos/ibc-go#5415), meaning chains that performed such upgrades before the guard could have broken ICA functionality. + +**What to Check**: +- [ ] Channel ordering is explicitly pinned when upgrading ibc-go versions +- [ ] ICA functionality is tested against counterparty chains running older ibc-go +- [ ] ICA channel states are monitored after upgrades + +**References**: +- [Issue](https://github.com/cosmos/ibc-go/issues/6714) +- [Fix](https://github.com/cosmos/ibc-go/pull/5415) + +--- + +### 15 ICA AUTHENTICATION MODULE MISSING OR MISCONFIGURED + +**Description**: ICA provides two entry points (`RegisterInterchainAccount` and `SendTx`) that are NOT exposed to end users directly. They must be gated by a custom "Authentication module" that controls who can use ICA. If the authentication module is missing, misconfigured, or overly permissive, any user may be able to register interchain accounts or submit transactions through them. The ICA spec (ICS-27) leaves the authentication module's design entirely to the chain developer. The informal systems audit noted: "The requirements and the API between the authentication and the ICA modules are not clearly specified." + +**What to Check**: +- [ ] A custom authentication module gates ICA usage +- [ ] `RegisterInterchainAccount` is restricted (governance, whitelisted addresses) +- [ ] Each `SendTx` call validates the caller is authorized for the specific interchain account +- [ ] ICA is NOT assumed to have built-in access control (it does not) + +--- + +### 16 IBC REENTRANCY / CEI VIOLATIONS + +**Description**: IBC's callback architecture (hooks, middleware) introduces external execution points between internal state operations. If a callback executes before internal state is finalized (e.g., packet commitment deleted), recursive re-entry is possible. This is a classic checks-effects-interactions (CEI) violation. The `OnTimeoutPacket` callback in ibc-go executed before the packet commitment was deleted — a malicious CosmWasm contract could re-enter via submessages since the commitment still existed. This class caused $6.4M in real losses and $150M+ at risk. + +**Known incidents**: ASA-2024-007 ($150M+ at risk, patched April 2024), Terra IBC hooks ($6.4M stolen, July 2024 — chain had accidentally reintroduced the patched vulnerability during an upgrade) + +**Affected versions**: ibc-go < v4.6.0, < v5.4.0, < v6.3.0, < v7.4.0, < v8.2.0 + +**Detection Patterns**: +```go +// VULNERABLE: Callback before state finalization (CEI violation) +func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet) error { + // 1. CHECK: Verify packet commitment exists + commitment := k.GetPacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) + + // 2. INTERACTION: Execute callback (ibc-hooks invokes CosmWasm contract) + // Malicious contract returns another MsgTimeout as submessage + // Commitment still exists → re-entry succeeds! + k.callbacks.OnTimeoutPacket(ctx, packet) + + // 3. EFFECT: Delete commitment (too late — already re-entered) + k.DeletePacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) + + return nil +} +``` + +```go +// SECURE: Effects before interactions (CEI pattern) +func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet) error { + // 1. CHECK + commitment := k.GetPacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) + + // 2. EFFECT: Delete commitment BEFORE callback + k.DeletePacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence) + + // 3. INTERACTION: Now callback cannot re-enter (commitment gone) + k.callbacks.OnTimeoutPacket(ctx, packet) + + return nil +} +``` + +**What to Check**: +- [ ] ibc-go version is patched (>= v4.6.0, v5.4.0, v6.3.0, v7.4.0, v8.2.0) +- [ ] ALL IBC callbacks (OnRecvPacket, OnAcknowledgementPacket, OnTimeoutPacket) follow CEI: state mutation BEFORE external callbacks +- [ ] Custom IBC middleware and hooks do NOT introduce new callback-before-finalization patterns +- [ ] If the chain has been upgraded, verify the patch was NOT accidentally reverted (Terra re-introduced the vulnerability during a June 2024 upgrade) +- [ ] CosmWasm-enabled chains with ibc-hooks middleware are at highest risk — CosmWasm submessages enable recursive re-entry +- [ ] Rate limiting (§12) is implemented as defense-in-depth even with patched ibc-go + +**Vulnerable chain requirements**: IBC-enabled + vulnerable ibc-go version + CosmWasm-enabled with permissionless uploads + ibc-hooks middleware wrapping ICS-20. + +**Two attack vectors**: +- **Native token theft**: Drain all tokens from ICS-20 escrow accounts +- **Infinite IBC token minting**: Mint unbacked IBC tokens, bridge them back to original chains + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `OnTimeoutPacket|OnAcknowledgementPacket|OnRecvPacket` — IBC callbacks +- `DeletePacketCommitment` — verify commitment deletion happens BEFORE callback invocation +- `ibc-hooks|ibchooks` — hooks middleware presence + +**References**: +- [Asymmetric Research — IBC Reentrancy Infinite Mint](https://blog.asymmetric.re/cosmos-ibc-reentrancy-infinite-mint/) +- [GHSA-j496-crgh-34mx](https://github.com/cosmos/ibc-go/security/advisories/GHSA-j496-crgh-34mx) +- [Terra IBC hooks exploit — $6.4M](https://www.theblock.co/post/308440/attacker-exploits-ibc-hooks-vulnerability-to-steal-tokens-on-terra-blockchain) + +--- + +### SUMMARY: MOST DANGEROUS PATTERNS + +| # | Pattern | Category | Known Incident / Reference | +|---|---------|----------|---------------------------| +| 1 | Untrusted IBC counterparty chain | Channel | GHSA-5jgq-x857-p8xw | +| 2 | Unsafe LookupModuleByPort/LookupModuleByChannel | Channel | ibc-go#71 | +| 3 | Missing IBC channel close validation | Channel | — | +| 4 | Incorrect IBC packet timeout handling | Channel | — | +| 5 | Incorrect IBC channel ordering type | Channel | — | +| 6 | IBC transfers breaking internal bookkeeping | Denom | SDK#20517 | +| 7 | Non-deterministic JSON in IBC ack middleware | Middleware | ISA-2025-001, ASA-2025-004 | +| 8 | PFM inverted escrow/mint (nonrefundable) | Middleware | PoC available | +| 9 | ICS-20 v2 multi-token partial receive inflation | Middleware | PoC available | +| 10 | Unpermissioned IBC channel opening | Channel | Precondition for ISA-2025-001 | +| 11 | IBC denom / voucher confusion | Denom | ibc-go#7848 | +| 12 | Missing IBC rate limiting | Defense | Would have limited $126M exploit | +| 13 | ICA host allow-all (`"*"`) configuration | ICA | Default genesis config | +| 14 | ICA channel ordering backwards compat | ICA | ibc-go#6714 | +| 15 | ICA authentication module missing | ICA | ICS-27 audit finding | +| 16 | IBC reentrancy / CEI violations | Reentrancy | ASA-2024-007, Terra $6.4M exploit | + +--- + +### References + +- **IBC-Go Security Advisories**: https://github.com/cosmos/ibc-go/security/advisories +- **IBC-Go Audit Reports**: https://docs.cosmos.network/ibc/v8.5.x/security-audits +- **Asymmetric Research**: "Cosmos IBC Reentrancy Infinite Mint" — https://blog.asymmetric.re/cosmos-ibc-reentrancy-infinite-mint/ +- **Jump Crypto**: "Huckleberry: IBC Event Hallucinations" — https://jumpcrypto.com/resources/huckleberry-ibc-event-hallucinations +- **Cosmos Forum**: IBC Security Advisories — https://forum.cosmos.network/c/security/ +- **Osmosis Rate Limits**: https://www.range.org/blog/ibc-rate-limits-introduction-and-state-of-the-art-1-3 diff --git a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/STATE_VULNERABILITY_PATTERNS.md b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/STATE_VULNERABILITY_PATTERNS.md new file mode 100644 index 00000000..95152752 --- /dev/null +++ b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/STATE_VULNERABILITY_PATTERNS.md @@ -0,0 +1,515 @@ +## State & Module Vulnerability Patterns + +> Updated for Cosmos SDK v0.53.x. State management, module interaction, and data handling patterns (§11-23) covering bookkeeping, bank keeper, pagination, events, transaction replay, governance, arithmetic, encoding, and deprecated modules. Referenced by [SKILL.md](../SKILL.md) checklist items via `§` anchors. + +### 11 BROKEN BOOKKEEPING + +**Description**: Custom internal accounting alongside `x/bank` module becomes inconsistent when direct token transfers bypass internal bookkeeping. + +**Detection Patterns**: +```go +// VULNERABLE: Internal bookkeeping separate from x/bank +type Keeper struct { + // Internal tracking of balances + userBalances map[string]sdk.Coins // NOT synchronized with x/bank! +} + +func (k Keeper) Deposit(ctx context.Context, user string, amount sdk.Coins) { + // Updates internal bookkeeping + k.userBalances[user] = k.userBalances[user].Add(amount...) + + // Also updates x/bank + k.bankKeeper.SendCoins(ctx, sender, moduleAccount, amount) +} + +// PROBLEM: Direct IBC transfer bypasses internal bookkeeping! +// User receives tokens via IBC -> x/bank updated but userBalances not updated +// Invariant violated: sum(userBalances) != bankKeeper.GetSupply() +``` + +**What to Check**: +- [ ] No custom balance tracking alongside x/bank +- [ ] OR custom tracking uses blocklist to prevent unexpected transfers +- [ ] Invariant checks compare internal accounting to x/bank +- [ ] Module accounts use SendEnabled parameter + +**Mitigation**: +```go +// OPTION 1: Use blocklist to prevent unexpected transfers +func (k Keeper) BeforeTokenTransfer(ctx context.Context, from, to string) error { + if !k.IsAuthorizedTransfer(ctx, from, to) { + return errors.New("direct transfers blocked") + } + return nil +} + +// OPTION 2: Use SendEnabled parameter +// In x/bank params, set SendEnabled = false for your token + +// OPTION 3: Don't maintain separate bookkeeping +// Use x/bank as source of truth, query when needed +func (k Keeper) GetUserBalance(ctx context.Context, user string) sdk.Coins { + addr := sdk.AccAddress(user) + return k.bankKeeper.GetAllBalances(ctx, addr) +} +``` + +**Invariant Testing**: +```go +func (k Keeper) InvariantCheck(ctx sdk.Context) error { + internalTotal := sdk.NewCoins() + k.IterateBalances(ctx, func(addr string, balance sdk.Coins) bool { + internalTotal = internalTotal.Add(balance...) + return false + }) + + moduleBalance := k.bankKeeper.GetAllBalances(ctx, k.moduleAccount) + if !internalTotal.Equal(moduleBalance) { + return fmt.Errorf("bookkeeping mismatch: internal=%v bank=%v", + internalTotal, moduleBalance) + } + return nil +} +``` + +**References**: building-secure-contracts/not-so-smart-contracts/cosmos/broken_bookkeeping + +--- + +### 12 BANK KEEPER & TOKEN SECURITY + +**Description**: Any module with access to `x/bank`'s keeper can mint, burn, and transfer tokens at will. The bank module provides blocklist, `SendEnabled`, and `SendRestriction` mechanisms to control this, but they must be configured correctly. + +**What to Check**: +- [ ] Modules with bank keeper access: [all methods are fully exposed](https://github.com/cosmos/cosmos-sdk/issues/7093) — review every usage for unauthorized mint/burn/transfer +- [ ] [Blocklist](https://docs.cosmos.network/master/modules/bank/02_keepers.html#blocklisting-addresses) prevents unexpected transfers to module accounts + - Addresses blocked during startup via bank keeper's argument. Default blocked addresses come from `maccPerms` via `BlockedAddresses()` + - `x/gov` module address explicitly **removed** from blocklist (receives proposal deposits) — verify custom chains make the same intentional exclusions + - Blocklist explicitly checked [whenever new transfer functionality is implemented](https://github.com/cosmos/cosmos-sdk/issues/8463#issuecomment-801046285) +- [ ] [`SendEnabled`](https://docs.cosmos.network/v0.45/modules/bank/05_params.html#parameters) prevents unexpected transfers of specific denominations. `DefaultSendEnabled` is true by default +- [ ] `SendRestriction` functions (`AppendSendRestriction`, `PrependSendRestriction`) reviewed if used. Order matters (Prepend vs Append). `ClearSendRestriction` removes ALL restrictions. Since v0.53.0, `SendCoins` checks restrictions BEFORE deducting coins (#24106, #24053) +- [ ] `SpendableCoins` used instead of `GetBalance` for accounts with [locked/vesting funds](https://github.com/umee-network/umee/issues/896) +- [ ] [Deflationary tokens](https://github.com/code-423n4/2021-08-gravitybridge-findings/issues/62) considered in transfer accounting +- [ ] If `x/mint` uses custom `MintFn` (via `keeper.WithMintFn`), no unbounded minting or inflation bypass. Old `InflationCalculationFn` must be `nil` + +**References**: [Desmos blocklist vulnerability](https://github.com/desmos-labs/desmos/compare/v0.15.3...v0.15.4) + +--- + +### 13 UNBOUNDED PAGINATION / QUERY DoS + +**Description**: gRPC query endpoints using `types/query.Paginate` accept a `Limit` field from clients with no enforced maximum. A malicious client can set `Limit: math.MaxUint64`, forcing the node to iterate over the entire store in a single request, causing OOM or extended blocking. The SDK's `DefaultLimit` is 100, but it is only applied when `Limit == 0` — any non-zero value is accepted as-is. Recent fix `d9d77304fd` addressed a related issue in `x/auth/tx` and `x/bank` genesis, but the core `types/query/pagination.go` still lacks a hard cap. + +**Detection Patterns**: +```go +// VULNERABLE: No max limit enforcement +func (k Keeper) AllRecords(ctx context.Context, req *types.QueryAllRecordsRequest) (*types.QueryAllRecordsResponse, error) { + store := k.storeService.OpenKVStore(ctx) + // Client controls req.Pagination.Limit with no upper bound! + results, pageRes, err := query.CollectionPaginate(ctx, k.Records, req.Pagination, + func(key string, value types.Record) (types.Record, error) { + return value, nil + }, + ) + // ... +} +``` + +**What to Check**: +- [ ] All gRPC query handlers enforce a maximum pagination limit +- [ ] Custom query endpoints that iterate stores have bounded iteration +- [ ] `CountTotal = true` requests are bounded (counting all records is expensive) +- [ ] Genesis export with large state is tested + +**Mitigation**: +```go +// SECURE: Enforce maximum pagination limit +const MaxPaginationLimit = 1000 + +func sanitizePagination(req *query.PageRequest) *query.PageRequest { + if req == nil { + req = &query.PageRequest{} + } + if req.Limit == 0 || req.Limit > MaxPaginationLimit { + req.Limit = MaxPaginationLimit + } + return req +} + +func (k Keeper) AllRecords(ctx context.Context, req *types.QueryAllRecordsRequest) (*types.QueryAllRecordsResponse, error) { + req.Pagination = sanitizePagination(req.Pagination) + // ... +} +``` + +--- + +### 14 EVENT OVERRIDE / SUPPRESSION + +**Description**: Cosmos SDK v0.53 introduced `EventManager.OverrideEvents()` which replaces all previously emitted events with a new set. If called in module code (especially in message handlers or ABCI hooks), it can silently suppress events emitted by earlier modules in the same block or by the SDK framework itself. This can break indexers, IBC relayers, and monitoring tools that depend on events. + +**Detection Patterns**: +```go +// VULNERABLE: Overriding events suppresses earlier module events +func (ms msgServer) Execute(ctx context.Context, msg *types.MsgExecute) (*types.MsgExecuteResponse, error) { + sdkCtx := sdk.UnwrapSDKContext(ctx) + // This wipes ALL events emitted so far in this transaction/block! + sdkCtx.EventManager().OverrideEvents(sdk.Events{myEvent}) + // Events from auth, bank, etc. are now gone +} +``` + +**What to Check**: +- [ ] No `OverrideEvents()` calls in module message handlers or ABCI methods +- [ ] If `OverrideEvents()` is used, verify it only operates on a child context's EventManager, not the parent +- [ ] Events from prior modules are not silently dropped +- [ ] IBC relayer and indexer compatibility verified after event changes + +**Mitigation**: +```go +// SECURE: Use EmitTypedEvent to add events, never OverrideEvents +func (ms msgServer) Execute(ctx context.Context, msg *types.MsgExecute) (*types.MsgExecuteResponse, error) { + sdkCtx := sdk.UnwrapSDKContext(ctx) + sdkCtx.EventManager().EmitTypedEvent(&types.EventExecute{...}) + // Preserves all existing events +} +``` + +--- + +### 15 UNORDERED TRANSACTION REPLAY + +**Description**: Cosmos SDK v0.53 supports unordered transactions (`TxBody.Unordered = true`) with timeout-timestamp-based replay protection instead of sequence numbers. These transactions use nonce tracking in `x/auth` with a `PreBlocker` that cleans up expired nonces. Vulnerabilities arise from: (1) insufficient timeout duration allowing replay after nonce cleanup, (2) chains that don't enable the `UnorderedTxDecorator` or set `maxTxTimeoutDuration` too high, (3) nonce storage DoS by flooding with unique unordered transactions. + +**Detection Patterns**: +```go +// VULNERABLE: Unordered tx support enabled without proper timeout limits +func NewAnteHandler(options HandlerOptions) sdk.AnteHandler { + decorators := []sdk.AnteDecorator{ + ante.NewSigVerificationDecorator( + options.AccountKeeper, + options.SignModeHandler, + // Missing: WithMaxUnorderedTxTimeoutDuration + // Missing: WithUnorderedTxGasCost + ), + } +} + +// VULNERABLE: Application logic assumes sequence-based ordering +func (k Keeper) ProcessUserAction(ctx context.Context, msg *types.MsgAction) error { + // With unordered txs, multiple actions from the same sender + // can execute in any order within the same block! + lastAction := k.GetLastAction(ctx, msg.Sender) + // This ordering assumption breaks with unordered txs +} +``` + +**What to Check**: +- [ ] `WithMaxUnorderedTxTimeoutDuration` is set to a reasonable value (default: 10 minutes) +- [ ] `WithUnorderedTxGasCost` is configured (default: 2240 gas) to prevent nonce flooding +- [ ] Application logic doesn't assume transaction ordering within a block +- [ ] PreBlocker includes `RemoveExpiredUnorderedNonces` cleanup +- [ ] State-dependent operations handle concurrent unordered transactions safely + +**Mitigation**: +```go +// SECURE: Configure unordered tx parameters +ante.NewSigVerificationDecorator( + options.AccountKeeper, + options.SignModeHandler, + ante.WithMaxUnorderedTxTimeoutDuration(10 * time.Minute), + ante.WithUnorderedTxGasCost(2240), +) + +// SECURE: Don't rely on transaction ordering +func (k Keeper) ProcessUserAction(ctx context.Context, msg *types.MsgAction) error { + // Use idempotent operations or explicit ordering fields + // instead of relying on sequence-based ordering +} +``` + +--- + +### 16 MESSAGE PRIORITY / PROPOSAL ORDERING + +**Description**: Missing prioritization of critical messages (oracle updates, emergency pause, governance) allows front-running and censorship during network congestion. In Cosmos SDK v0.50+ with ABCI 2.0, the block proposer controls transaction ordering via `PrepareProposal` and `ProcessProposal`, making this even more important. + +**Detection Patterns**: +```go +// VULNERABLE: No priority for critical oracle update in CheckTx +func (app *App) CheckTx(req *abci.CheckTxRequest) (*abci.CheckTxResponse, error) { + // All messages treated equally, oracle updates can be delayed + return app.BaseApp.CheckTx(req) +} + +// VULNERABLE: PrepareProposal does not prioritize critical messages +func (app *App) PrepareProposal(req *abci.PrepareProposalRequest) (*abci.PrepareProposalResponse, error) { + // Default ordering - no special treatment for emergency messages + return app.BaseApp.PrepareProposal(req) +} +``` + +**What to Check**: +- [ ] Oracle/price feed updates have high priority +- [ ] Emergency pause/circuit breaker messages prioritized +- [ ] Critical governance proposals prioritized +- [ ] `PrepareProposal` considers message priority when ordering transactions +- [ ] `ProcessProposal` validates that critical messages are not censored +- [ ] High fees required for priority transactions (prevent spam) + +**Mitigation**: +```go +// SECURE: Custom PrepareProposal that prioritizes critical messages +func (app *App) PrepareProposal(req *abci.PrepareProposalRequest) (*abci.PrepareProposalResponse, error) { + // Sort transactions by priority, putting critical messages first + prioritized := prioritizeTxs(req.Txs, req.MaxTxBytes) + return &abci.PrepareProposalResponse{Txs: prioritized}, nil +} + +func prioritizeTxs(txs [][]byte, maxBytes int64) [][]byte { + // Decode and classify transactions + // Place oracle updates and emergency messages first + // Then normal transactions by fee + // ... +} +``` + +**References**: building-secure-contracts/not-so-smart-contracts/cosmos/messages_priority + +--- + +### 17 GOVERNANCE SECURITY + +**Description**: Governance mechanisms can be abused for spam, manipulation, or fund locking. + +**What to Check**: +- [ ] If `MsgSubmitProposal` is registered, submission cost is non-zero. **`DefaultMinInitialDepositRatio` is zero** (`x/gov/types/v1/params.go:27`) — proposals can be submitted with zero deposit by default, creating spam risk. Check both `MinDeposit` AND `MinInitialDepositRatio` +- [ ] If `x/gov` uses custom `CalculateVoteResultsAndVotingPowerFn` (via `WithCustomCalculateVoteResultsAndVotingPowerFn`), verify the tally function correctly weights votes and handles edge cases (zero voting power, abstentions, validator vs delegator votes). A buggy tally function could manipulate governance outcomes +- [ ] If `x/circuit` module is enabled, the circuit breaker authority is reviewed — a compromised authority (or captured governance) can disable critical messages (e.g., unbonding, slashing), effectively locking user funds or preventing security-critical operations + +--- + +### 18 MODULE-SPECIFIC CHECKS + +**Description**: Known vulnerabilities, configuration pitfalls, and behavioral changes in specific SDK modules. + +**What to Check**: + +**x/distribution**: +- [ ] Historical rewards bounded — overflow can halt chain ([GHSA-p22h-3m2v-cmgh](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-p22h-3m2v-cmgh), fixed v0.53.3). Verify `incrementReferenceCount` and reward accumulation on older versions/forks + +**x/staking**: +- [ ] Delegator slashing bypass fixed ([GHSA-86h5-xcpx-cfqc](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-86h5-xcpx-cfqc), fixed v0.50.5) — verify slashing applies to all delegations including those created between infraction and slash execution +- [ ] Panics replaced with error returns in v0.53.0 ([#24391](https://github.com/cosmos/cosmos-sdk/pull/24391)) — custom code catching panics via `recover()` must handle returned errors instead + +**baseapp**: +- [ ] If `ValidateVoteExtensions` helper is used, verify chain runs >= v0.50.5. Original had bug ([GHSA-95rx-m9m5-m94v](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-95rx-m9m5-m94v)) where `currentHeight`/`chainID` args were misused. In fixed version they're ignored (read from context). In v0.51+ they're removed +- [ ] If `DisableBlockGasMeter` is enabled (`baseapp.DisableBlockGasMeter()`), per-block gas limits removed — only CometBFT's `max_bytes` applies. Removes DoS protection layer; only use if gas metered externally +- [ ] If Optimistic Execution enabled (`baseapp/oe/`), `FinalizeBlock` logic must not have side effects outside SDK state (external API calls, file writes) — speculative execution of rejected blocks cannot be rolled back +- [ ] `SetCheckTxHandler` can bypass standard ante handler validation if misused +- [ ] Custom `PrepareProposal` handlers must be deterministic in gas consumption and ordering. `ProcessProposal` rejections can cause consensus failures. Custom `SignerExtractionAdapter` must extract signers faithfully + +**x/epochs**: +- [ ] Epoch hooks (`BeforeEpochStart`, `AfterEpochEnd`) must not panic and must handle errors gracefully. `InvokeSetHooks` had a bug fixed in v0.53.2 ([#24770](https://github.com/cosmos/cosmos-sdk/pull/24770)). `CurrentEpochStartHeight` semantics fixed in v0.53.0 ([#24610](https://github.com/cosmos/cosmos-sdk/pull/24610)) + +**x/protocolpool**: +- [ ] If used alongside `x/distribution`, verify `CommunityPoolSpend`/`FundCommunityPool`/`CommunityPool` query handlers are NOT called (they return errors). Funds can get stuck if migration from distribution's community pool is incomplete + +**ABCIListener / StreamingManager**: +- [ ] If `SetStreamingManager` is used, listeners must not panic (halts chain), must not be excessively slow (synchronous during block processing), and must tolerate rejected blocks — especially with Optimistic Execution + +**Other**: +- [ ] No custom storage/cache implemented — [may fail to revert after failed transactions](https://github.com/iczc/billboard/tree/main/writeup#the-cache) +- [ ] [`errorsmod.Register`](https://pkg.go.dev/cosmossdk.io/errors) (from `cosmossdk.io/errors`) used for errors, not hand-rolled mechanisms +- [ ] [All defined messages actually registered in a handler](https://github.com/code-423n4/2021-08-gravitybridge-findings/issues/64) +- [ ] [CometBFT config/genesis.json not overwritten without validation](https://github.com/nomic-io/nomic/issues/104); [genesis.json save is atomic](https://github.com/crypto-org-chain/chain-main/issues/398) + +--- + +### 19 MODULE INITIALIZATION ORDERING + +**Description**: Cosmos SDK modules must be initialized in a specific order. Incorrect ordering can cause panics during genesis, missing state, or security bypasses. + +**What to Check**: +- [ ] `SetOrderPreBlockers`: `upgrade` module must be first. `auth` must also be in PreBlockers (required for unordered tx cleanup via `RemoveExpiredUnorderedNonces`) +- [ ] `SetOrderBeginBlockers` / `SetOrderEndBlockers`: `x/epochs` is in BeginBlockers and fires epoch hooks — modules depending on epoch events must come after `epochs`. Note: `capability` module has been removed from cosmos-sdk (moved to ibc-go) +- [ ] `SetOrderInitGenesis`: [`genutils` after `staking`](https://github.com/cosmos/cosmos-sdk/blob/cb6fea997cb416b6c9d3d1b47986bc464bd4bd49/simapp/app.go#L387-L388) (pool initialization); [`genutils` after `auth`](https://github.com/cosmos/cosmos-sdk/blob/cb6fea997cb416b6c9d3d1b47986bc464bd4bd49/simapp/app.go#L389) (param access) +- [ ] `SetOrderExportGenesis`: `protocolpool` must be exported before `bank` +- [ ] [Keepers passed to other keepers are initialized](https://github.com/osmosis-labs/osmosis/issues/665#issuecomment-995925080). Suspicious pattern: keeper initialized → uninitialized copy passed to another → first one assigned to `app.TheKeeper` + +--- + +### 20 ARITHMETIC ERRORS + +**Description**: Arithmetic bugs in financially-critical code — rounding errors, wrapping overflows, wrong operand selection, and missing sign/bound validation. Any deviation directly translates to fund loss. `math.LegacyDec` (formerly `sdk.Dec`) has precision issues and lacks associativity. + +**Detection Patterns**: + +#### Pattern 1: Rounding Errors (Precision Loss) +```go +// VULNERABLE: Division before multiplication (loses precision) +sharePrice := totalValue.Quo(totalShares) // Division first +userValue := sharePrice.Mul(userShares) // Multiplication second +// User gets less value due to rounding down in division + +// VULNERABLE: math.LegacyDec associativity issues +result1 := a.Mul(b).Quo(c) // (1 * 10) / 100 = 0.1 +result2 := a.Quo(c).Mul(b) // (1 / 100) * 10 = 0.1 (but different precision!) +// result1 != result2 due to precision handling + +// VULNERABLE: Repeated rounding favors users +for _, user := range users { + reward := totalReward.Quo(math.LegacyNewDec(int64(len(users)))) + k.MintReward(ctx, user, reward) +} +// Total minted > totalReward due to rounding up +``` + +#### Pattern 2: Wrong Operand in Calculation +```go +// VULNERABLE: Osmosis LP share miscalculation (Finding 9, $5M loss) +// MaximalExactRatioJoin multiplied shareRatio * userTokens +// instead of shareRatio * totalPoolLiquidity +shareRatio := userTokens.Quo(totalPoolLiquidity) +newShares := shareRatio.Mul(userTokens) // WRONG: should be shareRatio.Mul(totalShares) +// Users received 50% more LP shares than deserved +``` + +#### Pattern 3: Missing Sign/Bound Validation +```go +// VULNERABLE: Vesting Account Poisoning (Barberry, Finding 19) +func (msg MsgCreatePeriodicVestingAccount) ValidateBasic() error { + for _, period := range msg.VestingPeriods { + if period.Length <= 0 { + return errors.New("period length must be positive") + } + // Missing: validation that period.Amount coins are all positive! + // Negative amounts cause LockedCoins() to panic on withdrawal + } +} +``` + +**What to Check**: +- [ ] Multiplication before division pattern used +- [ ] Rounding direction favors protocol, not users +- [ ] No repeated rounding in loops +- [ ] Consistent calculation order across all operations +- [ ] Consider using integer arithmetic with scaling factor +- [ ] Operands in share/ratio calculations are verified against the formula (not swapped) +- [ ] CosmWasm wrapping overflow: see [COSMWASM_VULNERABILITY_PATTERNS.md §1](COSMWASM_VULNERABILITY_PATTERNS.md#1-wrapping-overflow-in-uint256512-arithmetic) +- [ ] All amount/coin fields validated for positive values (not just positive length/duration) — negative amounts in vesting periods cause permanent fund freeze +- [ ] Front-running of account creation reviewed — malicious vesting accounts can be created at victim addresses before the victim's first transaction + +**Mitigation**: +```go +// SECURE: Multiply before divide (preserves precision) +userValue := totalValue.Mul(userShares).Quo(totalShares) + +// SECURE: Round in favor of system +reward := totalReward.Mul(userShares).QuoTruncate(totalShares) + +// SECURE: Round up when calculating fees (users pay slightly more) +fee := amount.Mul(feeRate).QuoRoundUp(math.LegacyNewDec(10000)) + +// SECURE: Distribute with remainder handling +totalDistributed := math.LegacyZeroDec() +for i, user := range users { + var reward math.LegacyDec + if i == len(users)-1 { + reward = totalReward.Sub(totalDistributed) + } else { + reward = totalReward.Quo(math.LegacyNewDec(int64(len(users)))) + totalDistributed = totalDistributed.Add(reward) + } + k.MintReward(ctx, user, reward) +} + +// SECURE: Validate all coin amounts are positive +for _, period := range msg.VestingPeriods { + if !period.Amount.IsAllPositive() { + return errors.New("all period amounts must be positive") + } +} +``` + +**References**: +- building-secure-contracts/not-so-smart-contracts/cosmos/rounding_errors +- [Osmosis $5M exploit](https://www.certik.com/resources/blog/1o2gR0mG6wP83aHp5jzoor-osmosis-incident-analysis) (Finding 9) +- [Barberry / Vest in Peace](https://blog.fordefi.com/vest-in-peace-freezing-cosmos-account-funds-through-invalid-vesting-periods) (Finding 19) + +--- + +### 21 DATA VALIDATION & ENCODING + +**Description**: Encoding, validation, and type handling issues that can cause panics, state corruption, or chain halts. + +**What to Check**: +- [ ] [`Coins`/`DecCoins` always sorted and non-negative](https://github.com/cosmos/cosmos-sdk/blob/5f840be76aea654821cf4bb1a3de94757e09c0cc/types/dec_coin.go#L228-L235) — SDK relies on this for `Add`, `IsAnyGTE`, and genesis state loading +- [ ] [Unicode in `Coins` denominations handled correctly](https://github.com/cosmos/cosmos-sdk/issues/7046). [Differences between `Coin` vs `Coins` types understood](https://github.com/cosmos/cosmos-sdk/issues/7046) +- [ ] Since v0.53, `Coins.AmountOf()` no longer validates the denom and returns zero for missing denoms (no panic). `AmountOfNoDenomValidation` is deprecated (just calls `AmountOf`). Code that relied on `AmountOf` panicking on invalid denom must be updated +- [ ] `Add`, `Sub`, `Quo` on `LegacyDec` still panic — use error-returning variants or validate inputs first. Using pointer to `LegacyDec` (`*math.LegacyDec`) is error-prone: Dec consists of a single `big.Int` pointer, so pointer-to-pointer adds no performance benefit and risks nil-pointer dereference +- [ ] `cosmossdk.io/collections` framework: deterministic key encoding (custom key codecs produce consistent bytes), `IndexedMap` secondary indexes in sync, no `Remove`/`Set` during `Walk` iteration ([fix: #24460](https://github.com/cosmos/cosmos-sdk/pull/24460)), prefix/range queries used correctly +- [ ] [Encoding enforced as early as possible](https://github.com/axelarnetwork/axelar-core/issues/1148) — in msg server handler or protobuf definition (`string` in protobuf enforces UTF-8). Careful with [invalid UTF-8 string length](https://henvic.dev/posts/go-utf8/) +- [ ] [Invalid encoding in input does not halt chain](https://github.com/code-423n4/2021-08-gravitybridge-findings/issues/4) +- [ ] MustUnmarshal usage is safe +- [ ] `MsgUpdateParams` handlers validate parameter bounds — invalid params (rate > 100%, negative values, zero denominators) can be silently set via governance and cause issues in `BeginBlock`/`EndBlock` +- [ ] Default values are safe: + - [Configurable parameters are set explicitly or default values are reviewed](https://github.com/cosmos/cosmos-sdk/blob/5f840be76aea654821cf4bb1a3de94757e09c0cc/x/genutil/client/cli/migrate.go#L77-L83) + - Initial (genesis) system parameters are validated. Safety checks exist for parameters that get updated from "null" only after the first block ([Umee issue](https://github.com/umee-network/umee/commit/5372343a4f187864cc37f0c0d798dc7df3de16cc)) + - [`getter` methods do not return valid values on errors — they should panic or return nil](https://github.com/umee-network/umee/issues/861) +- [ ] Amino (`LegacyAmino`) only for backward compatibility (legacy JSON signing, CLI proposals). New modules use protobuf exclusively +- [ ] If Ethereum addresses used, NOT using `HexToAddress` ([does not verify format, ignores errors](https://pkg.go.dev/github.com/ethereum/go-ethereum/common#HexToAddress)) +- [ ] [`AnteHandler`s enforce non-zero fee](https://docs.cosmos.network/master/basics/gas-fees.html#introduction-to-gas-and-fees) to prevent spam. See [Cosmos discussion](https://github.com/cosmos/cosmos-sdk/discussions/8224) +- [ ] Parsing of empty `Coin(s)` considers any amount of extra whitespace — [relevant source](https://github.com/cosmos/cosmos-sdk/blob/v0.33.2/types/coin.go#L527-L534). For example, checking `msg.Funds == "" || msg.Funds == "0"` is incomplete; use the SDK's parsing functions instead of manual string comparison + +--- + +### 22 EVENTS & LOGGING (INCLUDING CacheContext EVENT LEAK) + +**Description**: Events are critical for indexers, relayers, and monitoring tools. Incorrect event handling breaks the ecosystem. A particularly dangerous pattern is the **CacheContext event leak**: when `CacheContext` is used to tentatively execute operations, state changes are correctly rolled back on failure, but events are still emitted to the parent context. Off-chain systems (bridges, indexers) trusting events without verifying on-chain state can be exploited. + +**Detection Patterns**: +```go +// VULNERABLE: CacheContext event leak (Huckleberry, Finding 4) +func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) exported.Acknowledgement { + cacheCtx, writeFn := ctx.CacheContext() + + // Execute in cached context — emits events to cacheCtx + err := k.processPacket(cacheCtx, packet) + if err != nil { + // State is rolled back (writeFn not called) + // BUT events from cacheCtx are still visible to parent context! + return channeltypes.NewErrorAcknowledgement(err) + } + writeFn() + return channeltypes.NewResultAcknowledgement([]byte{1}) +} +// Bridge monitors see valid deposit events for transfers that never happened +// Attacker gets credited on the bridge without tokens actually moving +``` + +**What to Check**: +- [ ] Enough event logs to debug on-chain transactions +- [ ] Events NOT generated for failed transactions — [Huckleberry: IBC Event Hallucinations](https://jumpcrypto.com/writing/huckleberry-ibc-event-hallucinations/) +- [ ] `CacheContext` usage reviewed: events emitted in cached context are visible even if `writeFn` is never called — state rolls back but events persist +- [ ] Off-chain systems (bridges, indexers, relayers) do NOT rely solely on events without verifying on-chain state +- [ ] Custom IBC `OnRecvPacket` that returns error ack does not emit misleading deposit/transfer events +- [ ] No log injection possible + +**References**: +- [Jump Crypto — Huckleberry: IBC Event Hallucinations](https://jumpcrypto.com/writing/huckleberry-ibc-event-hallucinations/) (Finding 4) + +--- + +### 23 DEPRECATED MODULES + +> As of v0.53.5+. Flag usage and advise migration. + +- **`x/crisis`** (Invariant type) — deprecated, will be removed. Use simulations (`simsx` framework in v0.53+) and property-based tests instead +- **`x/nft`** — deprecated ([#24575](https://github.com/cosmos/cosmos-sdk/pull/24575)), moving to [cosmos-legacy](https://github.com/cosmos/cosmos-legacy) +- **`x/group`** — deprecated ([#24571](https://github.com/cosmos/cosmos-sdk/pull/24571)), moving to cosmos-legacy. EndBlocker had chain-halt bugs ([GHSA-47ww-ff84-4jrg](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-47ww-ff84-4jrg), [GHSA-x5vx-95h7-rv4p](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-x5vx-95h7-rv4p), fixed v0.50.12/v0.53.0) +- **`x/params`** — deprecated, modules use direct keeper param storage via `collections` +- **`ValidateBasic()`** — deprecated and facultative (via `HasValidateBasic` interface). Validation belongs in msg server handlers +- **ed25519 tx signing** — added in v0.53.0 ([#24030](https://github.com/cosmos/cosmos-sdk/pull/24030)). Chains should explicitly decide which key types to permit. Unlike secp256k1, ed25519 is not compatible with Ledger hardware wallets without custom apps + +--- diff --git a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/VULNERABILITY_PATTERNS.md b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/VULNERABILITY_PATTERNS.md index 7bd9c386..22126e85 100644 --- a/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/VULNERABILITY_PATTERNS.md +++ b/plugins/building-secure-contracts/skills/cosmos-vulnerability-scanner/resources/VULNERABILITY_PATTERNS.md @@ -1,62 +1,8 @@ -## 6. Vulnerability Checklist (9 Patterns) +## Core Vulnerability Patterns & Audit Checks -### 6.1 INCORRECT GetSigners() ⚠️ CRITICAL +> Updated for Cosmos SDK v0.53.x. Core vulnerability patterns (§1-9) covering non-determinism, ABCI, signers, validation, handlers, and ante security. For state/module patterns see [STATE_VULNERABILITY_PATTERNS.md](STATE_VULNERABILITY_PATTERNS.md); for advanced patterns see [ADVANCED_VULNERABILITY_PATTERNS.md](ADVANCED_VULNERABILITY_PATTERNS.md). Referenced by [SKILL.md](../SKILL.md) checklist items via `§` anchors. -**Description**: Mismatch between address returned by `GetSigners()` and address actually used in handler allows unauthorized actions via signer impersonation. - -**Detection Patterns**: -```go -// VULNERABLE: GetSigners returns one address, handler uses different field -type MsgExample struct { - Signer string // Returned by GetSigners() - Author string // Actually used in handler -} - -func (msg MsgExample) GetSigners() []sdk.AccAddress { - return []sdk.AccAddress{sdk.AccAddress(msg.Signer)} -} - -// Handler uses Author instead! -func handleMsgExample(ctx sdk.Context, msg MsgExample) error { - // WRONG: Using msg.Author instead of verified signer - return keeper.DoAction(ctx, msg.Author, msg.Data) -} -``` - -**What to Check**: -- [ ] `GetSigners()` returns address that is actually used in handler -- [ ] No multiple address fields in message (signer, author, owner, from, etc.) -- [ ] Handler only uses addresses from `GetSigners()` -- [ ] User-provided addresses not stored without validation - -**Mitigation**: -```go -// SECURE: Single address field, used consistently -type MsgExample struct { - Signer string // Only address field -} - -func (msg MsgExample) GetSigners() []sdk.AccAddress { - return []sdk.AccAddress{sdk.AccAddress(msg.Signer)} -} - -func handleMsgExample(ctx sdk.Context, msg MsgExample) error { - // Use the verified signer address - signers := msg.GetSigners() - return keeper.DoAction(ctx, signers[0], msg.Data) -} -``` - -**Testing**: -- Unit test with different signer/author values -- Verify only GetSigners() address has authorization -- Sanity tests for all message types - -**References**: building-secure-contracts/not-so-smart-contracts/cosmos/incorrect_signer - ---- - -### 4.2 NON-DETERMINISM ⚠️ CRITICAL - CHAIN HALT +### 1 NON-DETERMINISM **Description**: Non-deterministic code in consensus causes different validators to produce different state roots, halting the chain. This is the most severe Cosmos vulnerability. @@ -112,7 +58,7 @@ filepath.Walk() // Filesystem traversal order **What to Check**: - [ ] NO `range` over maps in any consensus code -- [ ] NO `int`, `uint`, `float32`, `float64` types (use `int32`, `int64`, `sdk.Int`, `sdk.Dec`) +- [ ] NO `int`, `uint`, `float32`, `float64` types (use `int32`, `int64`, `math.Int`, `math.LegacyDec`) - [ ] NO goroutines in message handlers or ABCI methods - [ ] NO `select` statements with multiple channels - [ ] NO `rand` package usage (use deterministic PRF) @@ -133,187 +79,138 @@ for _, k := range keys { keeper.ProcessAsset(ctx, k, assets[k]) } -// Option 2: Use ordered data structure -// Use sdk.KVStore with ordered iteration +// Option 2: Use ordered data structure (sdk.KVStore with ordered iteration) // SECURE: Platform-independent types -var amount int64 // Explicit 64-bit -var amount sdk.Int // Arbitrary precision integer -var price sdk.Dec // Decimal type for consensus +var amount int64 // Explicit 64-bit +var amount math.Int // Arbitrary precision integer (cosmossdk.io/math) +var price math.LegacyDec // Decimal type for consensus (cosmossdk.io/math) // SECURE: Use block time, not system time timestamp := ctx.BlockTime() // Deterministic height := ctx.BlockHeight() // Deterministic ``` -**Tool Detection**: -```bash -# Use CodeQL custom rules -codeql database create --language=go -codeql query run cosmos-non-determinism.ql - -# Look for patterns -grep -r "range.*map\[" x/ -grep -r "go func" x/ -grep -r "time.Now()" x/ -grep -r "float64\|float32" x/ -``` +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `range.*map\[` — map iteration +- `go func` — goroutines +- `time\.Now\(\)` — system time +- `float64|float32` — floating point types +- `\bint\b|\buint\b` — platform-dependent integer types (exclude `int32`, `int64`, `uint32`, `uint64` results) **References**: building-secure-contracts/not-so-smart-contracts/cosmos/non_determinism --- -### 4.3 MESSAGES PRIORITY ⚠️ HIGH - -**Description**: Missing prioritization of critical messages (oracle updates, emergency pause, governance) allows front-running and censorship during network congestion. - -**Detection Patterns**: -```go -// VULNERABLE: No priority for critical oracle update -func (app *App) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx { - // All messages treated equally, oracle updates can be delayed - return app.BaseApp.CheckTx(req) -} +### 2 ADDITIONAL NON-DETERMINISM SOURCES -// VULNERABLE: Emergency pause has same priority as normal txs -// During congestion, pause message may not be included in time -``` +**Description**: Beyond the core non-determinism patterns in §1, these additional sources can cause consensus failures. **What to Check**: -- [ ] Oracle/price feed updates have high priority -- [ ] Emergency pause/circuit breaker messages prioritized -- [ ] Critical governance proposals prioritized -- [ ] CheckTx returns higher priority for critical message types -- [ ] High fees required for priority transactions (prevent spam) - -**Mitigation**: -```go -// SECURE: Prioritize critical messages in CheckTx -func (app *App) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx { - tx, err := app.txDecoder(req.Tx) - if err != nil { - return sdkerrors.ResponseCheckTx(err, 0, 0, app.trace) - } - - msgs := tx.GetMsgs() - priority := int64(0) - - for _, msg := range msgs { - switch msg.(type) { - case *oracle.MsgUpdatePrice: - // Verify sender is authorized oracle - if isAuthorizedOracle(msg.GetSigners()[0]) { - priority = 1000000 // Highest priority - } - case *crisis.MsgPause: - // Verify sender is admin - if isAdmin(msg.GetSigners()[0]) { - priority = 1000000 // Highest priority - } - } - } - - return abci.ResponseCheckTx{ - Code: 0, - Priority: priority, - // High priority messages pay higher fees to prevent spam - } -} -``` - -**References**: building-secure-contracts/not-so-smart-contracts/cosmos/messages_priority +- [ ] [`sort.Sort` is not stable](https://github.com/golang/go/issues/13884) — `sort.Stable` must be used when elements with equal keys must maintain insertion order. Same-order but different items may be sorted differently between Go versions +- [ ] Memory addresses — [even in non-committed storage, can cause indirect consensus failures](https://github.com/cosmos/cosmos-sdk/issues/11726#issuecomment-1108427164): + - [Conversions to strings do not output addresses](https://gitlab.com/thorchain/thornode/-/merge_requests/2640) (structs not used directly in `fmt.Printf`) + - [`%p` format string not used](https://facundo.dev/posts/2022/04/23/debug-stories-1-darwin-arm64-cosmos-sdk-builds-are-not-compatible-with-other-archs/) + - [Pointers not compared](https://github.com/OffchainLabs/nitro/blob/affaf25ceb9ff53ed27e1072221a659ed63ca61d/linter/pointercheck/pointer.go) +- [ ] `filepath.Ext` is platform-dependent +- [ ] Struct padding [(unavoidable with cgo)](https://github.com/golang/go/wiki/cgo#struct-alignment-issues) +- [ ] [Platform-dependent build tags](https://pkg.go.dev/go/build#hdr-Build_Constraints) (`//go:build`, `// +build`) +- [ ] [`math/big.Random` non-deterministic even with const seed](https://github.com/golang/go/issues/42701) +- [ ] [Adding elements to map while iterating](https://go.dev/ref/spec#For_range) +- [ ] No leaks from GC or nondeterministic runtime internals ([Agoric example](https://github.com/Agoric/agoric-sdk/issues/5000)) +- [ ] Parallel/concurrent tx execution (BlockSTM-style): modules must not have shared mutable state outside KV store +- [ ] Concurrency: goroutines, [`select` non-determinism](https://medium.com/@pedram.esmaeeli/a-pattern-for-overcoming-non-determinism-of-golang-select-statement-139dbe93db98), [synchronization with sleep](https://github.com/ligurio/semgrep-rules/blob/master/rules/non-determinism/go/synchronization-with-sleep.yaml). Use [race detector](https://go.dev/blog/race-detector) +- [ ] Floating point: read [here](https://gafferongames.com/post/floating_point_determinism/), [here](https://randomascii.wordpress.com/2013/07/16/floating-point-determinism/), [here](https://github.com/golang/go/issues/36536) + +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `sort\.Sort\b` — unstable sort +- `"%p"` — memory address format +- `filepath\.Ext` — platform-dependent +- `go func` — goroutines +- `select \{` — non-deterministic select --- -### 4.4 SLOW ABCI METHODS ⚠️ CRITICAL - CHAIN HALT +### 3 SLOW ABCI METHODS -**Description**: Computationally expensive `BeginBlocker` or `EndBlocker` with unbounded loops can exceed block time limits, halting the chain. +**Description**: Computationally expensive `BeginBlocker`, `EndBlocker`, or `PreBlocker` with unbounded loops can exceed block time limits, halting the chain. In SDK v0.50+, modules implement `appmodule.HasBeginBlocker` and `appmodule.HasEndBlocker` interfaces. `PreBlocker` is a new hook that runs before BeginBlock. Additionally, `appmodule.HasPrecommit` and `appmodule.HasPrepareCheckState` are lifecycle hooks that run after EndBlock and can also cause issues. Note: there are **two** EndBlock interfaces — `appmodule.HasEndBlocker` (returns `error`) and `module.HasABCIEndBlock` (returns `[]abci.ValidatorUpdate, error`, used by x/staking). **Detection Patterns**: ```go -// VULNERABLE: Unbounded loop in EndBlocker -func EndBlocker(ctx sdk.Context, k keeper.Keeper) { +// VULNERABLE: Unbounded loop in EndBlocker (v0.50+ style) +func (am AppModule) EndBlock(ctx context.Context) error { + sdkCtx := sdk.UnwrapSDKContext(ctx) // Iterates over ALL users - could be millions! - k.IterateAllUsers(ctx, func(user User) bool { - reward := k.CalculateReward(ctx, user) // Complex calculation - k.DistributeReward(ctx, user, reward) + am.keeper.IterateAllUsers(sdkCtx, func(user User) bool { + reward := am.keeper.CalculateReward(sdkCtx, user) + am.keeper.DistributeReward(sdkCtx, user, reward) return false }) + return nil } // VULNERABLE: Nested loops in BeginBlocker -func BeginBlocker(ctx sdk.Context, k keeper.Keeper) { - pools := k.GetAllPools(ctx) // 1000+ pools +func (am AppModule) BeginBlock(ctx context.Context) error { + sdkCtx := sdk.UnwrapSDKContext(ctx) + pools := am.keeper.GetAllPools(sdkCtx) // 1000+ pools for _, pool := range pools { - assets := k.GetPoolAssets(ctx, pool.ID) // 100+ assets each + assets := am.keeper.GetPoolAssets(sdkCtx, pool.ID) // 100+ assets each for _, asset := range assets { - k.UpdatePrice(ctx, pool, asset) // Expensive calculation + am.keeper.UpdatePrice(sdkCtx, pool, asset) // Expensive calculation } } + return nil } -// VULNERABLE: Unbounded state iteration -func (k Keeper) ProcessExpiredOrders(ctx sdk.Context) { - // No limit on number of orders processed per block! - k.IterateExpiredOrders(ctx, func(order Order) bool { - k.CancelOrder(ctx, order) - return false // Processes ALL expired orders - }) +// VULNERABLE: Unbounded PreBlocker +func (am AppModule) PreBlock(ctx context.Context) error { + // No limit on processing! + am.keeper.ProcessAllPending(ctx) + return nil } ``` **What to Check**: -- [ ] BeginBlocker has bounded computational complexity -- [ ] EndBlocker has bounded computational complexity +- [ ] `BeginBlock` has bounded computational complexity +- [ ] `EndBlock` has bounded computational complexity (both `HasEndBlocker` and `HasABCIEndBlock` variants) +- [ ] `PreBlock` (new in v0.50) has bounded computational complexity +- [ ] `Precommit` and `PrepareCheckState` hooks have bounded complexity - [ ] NO nested loops over unbounded collections - [ ] NO iterations over all users/pools/assets - [ ] Batch operations have size limits +- [ ] Normal (non-malicious) transactions cannot exceed CometBFT's `max_bytes` or `max_gas` block limits — a single large but valid message (e.g., governance proposal with many updates, batch operation) that fills an entire block prevents other transactions from being included and can degrade liveness - [ ] Stress tests with maximum expected data **Mitigation**: ```go -// SECURE: Process limited batch per block -func EndBlocker(ctx sdk.Context, k keeper.Keeper) { +// SECURE: Process limited batch per block (v0.50+ style) +func (am AppModule) EndBlock(ctx context.Context) error { + sdkCtx := sdk.UnwrapSDKContext(ctx) maxProcessed := 100 // Process max 100 users per block - iterator := k.GetUnprocessedUsers(ctx) + iterator := am.keeper.GetUnprocessedUsers(sdkCtx) defer iterator.Close() count := 0 for ; iterator.Valid() && count < maxProcessed; iterator.Next() { - user := k.UnmarshalUser(iterator.Value()) - reward := k.CalculateReward(ctx, user) - k.DistributeReward(ctx, user, reward) - k.MarkProcessed(ctx, user) + user := am.keeper.UnmarshalUser(iterator.Value()) + reward := am.keeper.CalculateReward(sdkCtx, user) + am.keeper.DistributeReward(sdkCtx, user, reward) + am.keeper.MarkProcessed(sdkCtx, user) count++ } // Remaining users processed in subsequent blocks -} - -// SECURE: Limit nested iterations -func BeginBlocker(ctx sdk.Context, k keeper.Keeper) { - // Process only active pools (limited set) - activePools := k.GetActivePools(ctx) // Max 50 pools - for _, pool := range activePools { - // Process only top assets (limited set) - topAssets := k.GetTopPoolAssets(ctx, pool.ID, 10) - for _, asset := range topAssets { - k.UpdatePrice(ctx, pool, asset) - } - } + return nil } ``` **Testing**: ```go -// Benchmark ABCI methods func BenchmarkEndBlocker(b *testing.B) { - // Test with maximum expected state ctx := setupMaximumState() // 1M users, 10K pools, etc. - b.ResetTimer() for i := 0; i < b.N; i++ { - EndBlocker(ctx, keeper) + am.EndBlock(ctx) } // Must complete in < 1 second } @@ -323,42 +220,53 @@ func BenchmarkEndBlocker(b *testing.B) { --- -### 4.5 ABCI METHODS PANIC ⚠️ CRITICAL - CHAIN HALT +### 4 ABCI METHODS PANIC -**Description**: Unexpected panics in `BeginBlocker` or `EndBlocker` immediately stop the blockchain. Many Cosmos SDK types panic on invalid operations. +**Description**: Unexpected panics in `BeginBlock`, `EndBlock`, `PreBlock`, `Precommit`, or `PrepareCheckState` immediately stop the blockchain. Many Cosmos SDK types panic on invalid operations. In SDK v0.50+, math types have moved to `cosmossdk.io/math` and x/params is deprecated in favor of direct keeper param storage, but panic-prone operations remain. Note: `ValidateBasic()` is now deprecated and facultative (validation should happen in `msg_server.go`), making it more likely that invalid inputs reach panic-prone code paths. **Detection Patterns**: #### Pattern 1: Panic-Prone Coin Operations ```go // VULNERABLE: NewCoins panics on invalid coins -func EndBlocker(ctx sdk.Context, k keeper.Keeper) { +func (am AppModule) EndBlock(ctx context.Context) error { + sdkCtx := sdk.UnwrapSDKContext(ctx) // Panics if amount is negative or denom is invalid! - coins := sdk.NewCoins(sdk.NewCoin(userDenom, userAmount)) - k.MintCoins(ctx, coins) + coins := sdk.NewCoins(sdk.NewCoin(userDenom, math.NewInt(userAmount))) + am.keeper.MintCoins(sdkCtx, coins) + return nil } // VULNERABLE: Coin arithmetic panics -reward := sdk.NewCoin("uatom", sdk.NewInt(-100)) // PANIC: negative amount +reward := sdk.NewCoin("uatom", math.NewInt(-100)) // PANIC: negative amount ``` -#### Pattern 2: Dec/Int Operations +#### Pattern 2: Math Operations ```go -// VULNERABLE: NewDec panics on invalid string -func BeginBlocker(ctx sdk.Context, k keeper.Keeper) { - price := sdk.NewDec(priceString) // PANIC if priceString is invalid! +// VULNERABLE: math.LegacyNewDecFromStr panics on invalid string +func (am AppModule) BeginBlock(ctx context.Context) error { + price, err := math.LegacyNewDecFromStr(priceString) // Returns error, but... + // If using the non-error variant: + price := math.LegacyMustNewDecFromStr(priceString) // PANIC if invalid! } // VULNERABLE: Division by zero -ratio := amount.Quo(sdk.ZeroInt()) // PANIC! +ratio := amount.Quo(math.ZeroInt()) // PANIC! + +// PITFALL: Using pointer to Dec is error-prone +var price *math.LegacyDec // Unnecessary pointer — LegacyDec is already a single pointer internally +// Dec consists of a single big.Int pointer — pointer-to-pointer adds indirection with no performance benefit +// and introduces nil-pointer dereference risk ``` -#### Pattern 3: SetParamSet +#### Pattern 3: Direct Param Storage (replaces SetParamSet) ```go -// VULNERABLE: SetParamSet panics on validation failure -func (k Keeper) UpdateParams(ctx sdk.Context, params Params) { - // PANIC if params are invalid! - k.paramSpace.SetParamSet(ctx, ¶ms) +// VULNERABLE: Setting params without validation +// In v0.50+, modules store params directly in keeper state +func (k Keeper) SetParams(ctx context.Context, params Params) error { + // If params are not validated before storage, invalid state can cause + // panics later when params are read and used in calculations + return k.Params.Set(ctx, params) } ``` @@ -372,307 +280,207 @@ func processValidators(validators []Validator) { **What to Check**: - [ ] All `sdk.NewCoins()`, `sdk.NewCoin()` calls validated -- [ ] All `sdk.NewDec()`, `sdk.NewInt()` calls validated +- [ ] All `math.LegacyNewDecFromStr()`, `math.NewIntFromString()` use error-returning variants +- [ ] No `Must*` variants (`LegacyMustNewDecFromStr`, `MustNewDecFromStr`) in consensus code - [ ] Division operations check for zero divisor -- [ ] `SetParamSet` called only with validated params +- [ ] Params validated before storage (even with direct keeper storage) - [ ] Array/slice access has bounds checking - [ ] User input validated before use in panic-prone operations **Mitigation**: ```go // SECURE: Validate before panic-prone operations -func EndBlocker(ctx sdk.Context, k keeper.Keeper) { - // Validate denom and amount +func (am AppModule) EndBlock(ctx context.Context) error { + sdkCtx := sdk.UnwrapSDKContext(ctx) + if err := sdk.ValidateDenom(userDenom); err != nil { - ctx.Logger().Error("invalid denom", "error", err) - return + sdkCtx.Logger().Error("invalid denom", "error", err) + return nil } if userAmount.IsNegative() { - ctx.Logger().Error("negative amount") - return + sdkCtx.Logger().Error("negative amount") + return nil } coins := sdk.NewCoins(sdk.NewCoin(userDenom, userAmount)) - k.MintCoins(ctx, coins) + am.keeper.MintCoins(sdkCtx, coins) + return nil } // SECURE: Use safe constructors -func (k Keeper) UpdatePrice(ctx sdk.Context, priceStr string) error { - // Safe: Returns error instead of panicking - price, err := sdk.NewDecFromStr(priceStr) +func (k Keeper) UpdatePrice(ctx context.Context, priceStr string) error { + price, err := math.LegacyNewDecFromStr(priceStr) if err != nil { return err } - // Check for zero before division if divisor.IsZero() { return errors.New("division by zero") } ratio := amount.Quo(divisor) - return nil } - -// SECURE: Bounds checking -func processValidators(validators []Validator) { - if len(validators) == 0 { - return - } - top := validators[0] // Safe -} ``` -**Tool Detection**: -```bash -# Use CodeQL to find panic-prone operations -codeql query run find-unvalidated-sdk-operations.ql - -# Manual review -grep -r "sdk.NewDec\|sdk.NewInt\|sdk.NewCoins" x/ -grep -r "\.Quo\|\.Div" x/ -grep -r "SetParamSet" x/ -``` +**Grep Patterns** (use Grep tool with `glob: "*.go"` in `x/`): +- `MustNewDec|MustNewInt|LegacyMustNewDec` — panic-prone constructors +- `\.Quo|\.Div` — division operations (check for zero divisor) +- `math\.NewInt|math\.LegacyNewDec` — math constructors **References**: building-secure-contracts/not-so-smart-contracts/cosmos/abci_method_panics --- -### 4.6 BROKEN BOOKKEEPING ⚠️ HIGH +### 5 INCORRECT SIGNER ANNOTATION / MISMATCH -**Description**: Custom internal accounting alongside `x/bank` module becomes inconsistent when direct token transfers bypass internal bookkeeping. +**Description**: In Cosmos SDK v0.47+, `GetSigners() []sdk.AccAddress` no longer exists on message types. `sdk.Msg` is now just `proto.Message`. Signers are declared via the `cosmos.msg.v1.signer` proto annotation and resolved automatically by `x/tx/signing`. The vulnerability now manifests as a mismatch between the proto-annotated signer field and the field actually used for authorization in `msg_server.go`. **Detection Patterns**: -```go -// VULNERABLE: Internal bookkeeping separate from x/bank -type Keeper struct { - // Internal tracking of balances - userBalances map[string]sdk.Coins // NOT synchronized with x/bank! -} -func (k Keeper) Deposit(ctx sdk.Context, user string, amount sdk.Coins) { - // Updates internal bookkeeping - k.userBalances[user] = k.userBalances[user].Add(amount...) +#### Pattern 1: Proto annotation vs msg_server mismatch +```protobuf +// In tx.proto +message MsgTransferOwnership { + option (cosmos.msg.v1.signer) = "sender"; // "sender" is the verified signer - // Also updates x/bank - k.bankKeeper.SendCoins(ctx, sender, moduleAccount, amount) + string sender = 1; + string new_owner = 2; } - -// PROBLEM: Direct IBC transfer bypasses internal bookkeeping! -// User receives tokens via IBC -> x/bank updated but userBalances not updated -// Invariant violated: sum(userBalances) != bankKeeper.GetSupply() ``` -**What to Check**: -- [ ] No custom balance tracking alongside x/bank -- [ ] OR custom tracking uses blocklist to prevent unexpected transfers -- [ ] Invariant checks compare internal accounting to x/bank -- [ ] IBC transfers handled correctly -- [ ] Module accounts use SendEnabled parameter - -**Mitigation**: ```go -// OPTION 1: Use blocklist to prevent unexpected transfers -func (k Keeper) BeforeTokenTransfer(ctx sdk.Context, from, to string) error { - // Block all transfers except through our module - if !k.IsAuthorizedTransfer(ctx, from, to) { - return errors.New("direct transfers blocked") +// In msg_server.go - VULNERABLE: uses new_owner for authorization instead of sender +func (ms msgServer) TransferOwnership(ctx context.Context, msg *types.MsgTransferOwnership) (*types.MsgTransferOwnershipResponse, error) { + // BUG: authorizing based on new_owner, not the verified sender! + if !ms.IsAdmin(ctx, msg.NewOwner) { + return nil, errors.ErrUnauthorized } - return nil -} - -// OPTION 2: Use SendEnabled parameter -// In x/bank params, set SendEnabled = false for your token -// All transfers must go through your module - -// OPTION 3: Don't maintain separate bookkeeping -// Use x/bank as source of truth, query when needed -func (k Keeper) GetUserBalance(ctx sdk.Context, user string) sdk.Coins { - addr := sdk.AccAddress(user) - return k.bankKeeper.GetAllBalances(ctx, addr) + // ... } ``` -**Invariant Testing**: -```go -// Invariant: Internal accounting matches x/bank -func (k Keeper) InvariantCheck(ctx sdk.Context) error { - internalTotal := sdk.NewCoins() - for _, balance := range k.userBalances { - internalTotal = internalTotal.Add(balance...) - } - - moduleBalance := k.bankKeeper.GetAllBalances(ctx, k.moduleAccount) +#### Pattern 2: Wrong field annotated as signer +```protobuf +// VULNERABLE: signer annotation points to wrong field +message MsgExecute { + option (cosmos.msg.v1.signer) = "authority"; // Should be "sender" - if !internalTotal.IsEqual(moduleBalance) { - return fmt.Errorf("bookkeeping mismatch: internal=%v bank=%v", - internalTotal, moduleBalance) - } - return nil + string sender = 1; // actual tx signer + string authority = 2; // module authority, not tx signer } ``` -**References**: building-secure-contracts/not-so-smart-contracts/cosmos/broken_bookkeeping +#### Pattern 3: Multiple address fields with ambiguous signer +```protobuf +message MsgDelegate { + option (cosmos.msg.v1.signer) = "delegator"; ---- - -### 4.7 ROUNDING ERRORS ⚠️ MEDIUM - -**Description**: `sdk.Dec` type has precision issues and lacks associativity, causing rounding errors that can be exploited or cause incorrect calculations. - -**Detection Patterns**: -```go -// VULNERABLE: Division before multiplication (loses precision) -sharePrice := totalValue.Quo(totalShares) // Division first -userValue := sharePrice.Mul(userShares) // Multiplication second -// User gets less value due to rounding down in division - -// VULNERABLE: sdk.Dec associativity issues -a := sdk.NewDec(1) -b := sdk.NewDec(10) -c := sdk.NewDec(100) - -result1 := a.Mul(b).Quo(c) // (1 * 10) / 100 = 0.1 -result2 := a.Quo(c).Mul(b) // (1 / 100) * 10 = 0.1 (but different precision!) -// result1 != result2 due to precision handling - -// VULNERABLE: Repeated rounding favors users -for _, user := range users { - reward := totalReward.Quo(sdk.NewDec(len(users))) // Round each time - k.MintReward(ctx, user, reward) -} -// Total minted > totalReward due to rounding up + string delegator = 1; + string validator = 2; + string beneficiary = 3; // If handler uses this instead of delegator... +} ``` **What to Check**: -- [ ] Multiplication before division pattern used -- [ ] Rounding direction favors protocol, not users -- [ ] No repeated rounding in loops -- [ ] Consistent calculation order across all operations -- [ ] Consider using integer arithmetic with scaling factor +- [ ] `cosmos.msg.v1.signer` annotation field matches the field used for authorization in `msg_server.go` +- [ ] No multiple address fields where the wrong one is used for access control +- [ ] `msg_server.go` handler references `msg.Signer` (or whatever the annotated field is), not another address field +- [ ] Custom `GetSigners` overrides via `DefineCustomGetSigners` are correct **Mitigation**: +```protobuf +// SECURE: Single clear signer field +message MsgExample { + option (cosmos.msg.v1.signer) = "sender"; + string sender = 1; // Only address field used for authorization +} +``` + ```go -// SECURE: Multiply before divide (preserves precision) -userValue := totalValue.Mul(userShares).Quo(totalShares) -// Full precision maintained until final division - -// SECURE: Round in favor of system -// When distributing rewards, round down (users get slightly less) -reward := totalReward.Mul(userShares).QuoTruncate(totalShares) - -// When calculating fees, round up (users pay slightly more) -fee := amount.Mul(feeRate).QuoCeil(sdk.NewDec(10000)) - -// SECURE: Distribute with remainder handling -totalDistributed := sdk.ZeroDec() -for i, user := range users { - if i == len(users)-1 { - // Last user gets remainder to ensure sum is exact - reward = totalReward.Sub(totalDistributed) - } else { - reward = totalReward.Quo(sdk.NewDec(len(users))) - totalDistributed = totalDistributed.Add(reward) +// SECURE: msg_server uses the annotated signer field +func (ms msgServer) Example(ctx context.Context, msg *types.MsgExample) (*types.MsgExampleResponse, error) { + // Use msg.Sender - the field declared in cosmos.msg.v1.signer + if err := ms.authorize(ctx, msg.Sender); err != nil { + return nil, err } - k.MintReward(ctx, user, reward) + // ... } ``` -**References**: building-secure-contracts/not-so-smart-contracts/cosmos/rounding_errors +**Testing**: +- Unit test with crafted messages where signer field != other address fields +- Verify only the annotated signer has authorization +- Integration test all message types + +**References**: building-secure-contracts/not-so-smart-contracts/cosmos/incorrect_signer --- -### 4.8 UNREGISTERED MESSAGE HANDLER ⚠️ MEDIUM (Legacy Issue) +### 6 MISSING MSG_SERVER VALIDATION (ValidateBasic Deprecation) -**Description**: Message types defined in proto but not registered in `NewHandler` function cause messages to be accepted but silently ignored (pre-Cosmos SDK v0.47). +**Description**: In Cosmos SDK v0.50+, `ValidateBasic()` is deprecated and facultative — the SDK no longer requires messages to implement it. The recommended pattern is to validate inputs directly in `msg_server.go` handlers. However, if a module removes `ValidateBasic()` without adding equivalent validation in the handler, invalid messages reach state-changing code without checks. **Detection Patterns**: ```go -// VULNERABLE: Message defined but not in handler (legacy Msg Service) -// In types/msgs.proto -message MsgWithdraw { - string sender = 1; - string amount = 2; -} - -// In handler.go -func NewHandler(k keeper.Keeper) sdk.Handler { - return func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { - switch msg := msg.(type) { - case *types.MsgDeposit: - return handleMsgDeposit(ctx, k, msg) - // Missing: case *types.MsgWithdraw - default: - return nil, sdkerrors.ErrUnknownRequest - } - } +// VULNERABLE: ValidateBasic removed but no validation in handler +// types/msgs.go - ValidateBasic was here but got removed during migration + +// msg_server.go - No validation of inputs +func (ms msgServer) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { + // No check on msg.Authority + // No check on msg.Params fields + return &types.MsgUpdateParamsResponse{}, ms.Params.Set(ctx, msg.Params) } ``` -**What to Check**: -- [ ] Using Cosmos SDK v0.47+ with automatic handler registration -- [ ] OR all message types in proto have corresponding handler case -- [ ] Integration tests call all message types -- [ ] CI checks for unregistered messages - -**Mitigation**: ```go -// OPTION 1: Use modern SDK (v0.47+) - handlers auto-registered -// In msg_server.go -type msgServer struct { - Keeper -} - -func (s msgServer) Deposit(ctx context.Context, msg *types.MsgDeposit) (*types.MsgDepositResponse, error) { - // Handler automatically registered via protobuf service -} - -func (s msgServer) Withdraw(ctx context.Context, msg *types.MsgWithdraw) (*types.MsgWithdrawResponse, error) { - // Handler automatically registered +// VULNERABLE: HasValidateBasic implemented but handler also needs validation +// types/msgs.go +func (msg *MsgTransfer) ValidateBasic() error { + // Only checks basic format — NOT authorization or business logic + return nil } -// OPTION 2: Verify all messages registered (legacy) -func NewHandler(k keeper.Keeper) sdk.Handler { - return func(ctx sdk.Context, msg sdk.Msg) (*sdk.Result, error) { - switch msg := msg.(type) { - case *types.MsgDeposit: - return handleMsgDeposit(ctx, k, msg) - case *types.MsgWithdraw: // Ensure all messages present! - return handleMsgWithdraw(ctx, k, msg) - default: - return nil, sdkerrors.ErrUnknownRequest - } - } +// msg_server.go - Relies entirely on ValidateBasic, no handler-level checks +func (ms msgServer) Transfer(ctx context.Context, msg *types.MsgTransfer) (*types.MsgTransferResponse, error) { + // Missing: check that msg.Amount is positive + // Missing: check that sender has sufficient funds + // Missing: authorization check against cosmos.msg.v1.signer + return ms.executeTransfer(ctx, msg) } ``` -**Testing**: +**What to Check**: +- [ ] Every `msg_server.go` handler validates all input fields (don't rely on ValidateBasic) +- [ ] Authority/signer checks are in the handler, not just in ValidateBasic +- [ ] Params are validated before `Params.Set()` calls +- [ ] Numeric inputs checked for negative/zero/overflow before use +- [ ] Address strings validated before conversion + +**Mitigation**: ```go -// Integration test for all message types -func TestAllMessageTypes(t *testing.T) { - // Get all message types from proto - messageTypes := getAllProtoMessageTypes() - - for _, msgType := range messageTypes { - // Verify message can be submitted and processed - result, err := app.DeliverTx(ctx, msgType) - require.NoError(t, err) - require.NotNil(t, result) +// SECURE: Full validation in msg_server.go handler +func (ms msgServer) UpdateParams(ctx context.Context, msg *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { + if ms.authority != msg.Authority { + return nil, errorsmod.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", ms.authority, msg.Authority) + } + if err := msg.Params.Validate(); err != nil { + return nil, err } + return &types.MsgUpdateParamsResponse{}, ms.Params.Set(ctx, msg.Params) } ``` -**References**: building-secure-contracts/not-so-smart-contracts/cosmos/message_handler_missing - --- -### 4.9 MISSING ERROR HANDLER ⚠️ HIGH +### 7 MISSING ERROR HANDLER **Description**: Ignoring error return values from keeper methods (especially `bankKeeper.SendCoins`) allows invalid operations to silently succeed. **Detection Patterns**: ```go // VULNERABLE: Ignored error from SendCoins -func (k Keeper) Withdraw(ctx sdk.Context, user string, amount sdk.Coins) { +func (k Keeper) Withdraw(ctx context.Context, user string, amount sdk.Coins) { // Error ignored! Withdrawal appears successful even if SendCoins fails k.bankKeeper.SendCoins(ctx, moduleAccount, user, amount) @@ -681,7 +489,7 @@ func (k Keeper) Withdraw(ctx sdk.Context, user string, amount sdk.Coins) { } // VULNERABLE: Deferred error handling too late -func (k Keeper) ProcessBatch(ctx sdk.Context, txs []Transaction) { +func (k Keeper) ProcessBatch(ctx context.Context, txs []Transaction) { for _, tx := range txs { err := k.ProcessTransaction(ctx, tx) // Error not checked, continues processing! @@ -699,19 +507,16 @@ func (k Keeper) ProcessBatch(ctx sdk.Context, txs []Transaction) { **Mitigation**: ```go // SECURE: Check all errors -func (k Keeper) Withdraw(ctx sdk.Context, user string, amount sdk.Coins) error { - // Check error from SendCoins +func (k Keeper) Withdraw(ctx context.Context, user string, amount sdk.Coins) error { if err := k.bankKeeper.SendCoins(ctx, moduleAccount, user, amount); err != nil { - return err // Withdrawal failed, no state change + return err } - - // Only update state if SendCoins succeeded k.DecrementBalance(ctx, user, amount) return nil } // SECURE: Stop processing on first error -func (k Keeper) ProcessBatch(ctx sdk.Context, txs []Transaction) error { +func (k Keeper) ProcessBatch(ctx context.Context, txs []Transaction) error { for _, tx := range txs { if err := k.ProcessTransaction(ctx, tx); err != nil { return fmt.Errorf("transaction failed: %w", err) @@ -726,15 +531,123 @@ func (k Keeper) ProcessBatch(ctx sdk.Context, txs []Transaction) error { # .golangci.yml linters: enable: - - errcheck # Detect unchecked errors - - goerr113 # Error handling rules + - errcheck + - goerr113 linters-settings: errcheck: - check-blank: true # Flag _ = err + check-blank: true check-type-assertions: true ``` **References**: building-secure-contracts/not-so-smart-contracts/cosmos/missing_error_handler --- + +### 8 UNREGISTERED MESSAGE HANDLER (Legacy Only) + +**Description**: In Cosmos SDK v0.50+, all modules use protobuf service-based message handling (`msg_server.go`). The legacy `NewHandler`/`sdk.Handler` switch-case pattern no longer exists in the SDK. Messages are auto-registered when the module implements `RegisterServices`. + +This pattern is **only relevant for chains still on SDK < v0.47** or chains that have not migrated legacy handlers. + +**When This Still Applies**: +- Chains running Cosmos SDK < v0.47 +- Custom modules that haven't migrated to protobuf services +- Forked SDK versions with legacy handler patterns + +**Modern Equivalent Issue**: +Even with protobuf service registration, a related issue can occur: +```go +// msg_server.go - Message type defined in proto but method not implemented +type msgServer struct { + Keeper +} + +var _ types.MsgServer = msgServer{} + +// If a method is declared in the proto service but the Go implementation +// is missing or returns a nil/empty response, the compiler will catch +// missing methods (unlike the legacy switch-case), but the implementation +// could still be a no-op stub. + +func (ms msgServer) Withdraw(ctx context.Context, msg *types.MsgWithdraw) (*types.MsgWithdrawResponse, error) { + // VULNERABLE: Stub implementation that does nothing + return &types.MsgWithdrawResponse{}, nil +} +``` + +**What to Check**: +- [ ] All proto service RPC methods have real implementations in `msg_server.go` +- [ ] No stub/no-op implementations that accept messages without processing +- [ ] Integration tests call all message types and verify state changes +- [ ] `RegisterServices` is called in the module's `AppModule` + +**References**: building-secure-contracts/not-so-smart-contracts/cosmos/message_handler_missing + +--- + +### 9 ANTEHANDLER SECURITY + +**Description**: AnteHandlers form the authentication and validation pipeline. Misconfigurations allow bypassing signature verification, fee checks, or authorization. + +**What to Check**: +- [ ] `SetUpContext` is the first decorator in `ChainAnteDecorators` +- [ ] `SetPubKeyDecorator` before all signature verification decorators (`ValidateSigCountDecorator`, `SigVerificationDecorator`) +- [ ] Manual message routing is NOT performed — it [bypasses AnteHandlers](https://github.com/axelarnetwork/axelar-core/issues/1145) +- [ ] Custom AnteHandlers handle [nested/embedded messages](https://jumpcrypto.com/bypassing-ethermint-ante-handlers/) inside `x/authz MsgExec` or `x/group` proposal execution. SDK enforces `MaxUnpackAnyRecursionDepth = 10` ([GHSA-8wcc-m6j2-qxvm](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-8wcc-m6j2-qxvm)) but semantic nesting (authz wrapping authz) must be handled by the ante handler itself +- [ ] `x/authz` grant creation reviewed for [privilege escalation (GHSA-4j93-fm92-rp4m)](https://github.com/cosmos/cosmos-sdk/security/advisories/GHSA-4j93-fm92-rp4m) — verify grants cannot circumvent blocked address restrictions; also check `x/feegrant`'s `GrantAllowance` and `x/auth/vesting`'s `CreatePeriodicVestingAccount` against `BlockedAddr` +- [ ] If `PostHandler` is set (`app.SetPostHandler`), its logic is reviewed — PostHandlers run after message execution and receive a `success` bool; errors can mask the original message result or cause unexpected state changes +- [ ] Unordered transactions (ADR-070): `SigVerificationDecorator` configured with `SigVerifyOptions` (timeout duration, gas cost) + +**Key Vulnerability**: Nested message bypass. An attacker wraps a message inside `MsgExec` (authz) or a group proposal. If the custom ante handler only inspects the outer message, inner messages skip validation entirely. + +```go +// VULNERABLE: AnteHandler only checks outer message type +func (d MyDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + for _, msg := range tx.GetMsgs() { + // Only checks msg type — does NOT recurse into MsgExec.Msgs! + if _, ok := msg.(*types.MsgDangerous); ok { + return ctx, errors.New("blocked") + } + } + return next(ctx, tx, simulate) +} +``` + +```go +// SECURE: Recurse into nested messages +func (d MyDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + for _, msg := range tx.GetMsgs() { + if err := d.checkMsg(msg); err != nil { + return ctx, err + } + } + return next(ctx, tx, simulate) +} + +func (d MyDecorator) checkMsg(msg sdk.Msg) error { + if _, ok := msg.(*types.MsgDangerous); ok { + return errors.New("blocked") + } + if exec, ok := msg.(*authz.MsgExec); ok { + for _, inner := range exec.Msgs { + var innerMsg sdk.Msg + if err := d.cdc.UnpackAny(inner, &innerMsg); err != nil { + return err + } + if err := d.checkMsg(innerMsg); err != nil { // Recurse + return err + } + } + } + return nil +} +``` + +**References**: [Jump Crypto — Bypassing Ethermint Ante Handlers](https://jumpcrypto.com/bypassing-ethermint-ante-handlers/) + +--- + +### 10 IBC SECURITY + +> IBC vulnerability patterns are covered separately in Step 4 of the scanning workflow. See [IBC_VULNERABILITY_PATTERNS.md](IBC_VULNERABILITY_PATTERNS.md). This section intentionally left as a placeholder to preserve numbering.