Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .github/workflows/migration_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ jobs:
steps:
- uses: actions/checkout@v5
with:
fetch-depth: 0
fetch-depth: 1
Comment on lines 59 to +61

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read-only verification: check whether any Dockerfile or build config copies the repo
# workspace in a way that could include .git / checkout credentials.
fd -a 'Dockerfile*' .
fd -a '.dockerignore' .

rg -n --hidden \
  -g 'Dockerfile*' \
  -g '.dockerignore' \
  -e 'COPY\s+\.\s' \
  -e 'ADD\s+\.\s' \
  -e '\.git' \
  -e 'persist-credentials' \
  -e 'actions/checkout' \
  .

Repository: evstack/ev-abci

Length of output: 385


Disable persisted checkout credentials in these jobs.

actions/checkout keeps the token in git config by default. These jobs do not need repo credentials after checkout, and leaving them enabled raises avoidable leak risk if the workspace is copied into a Docker context or cache, as this workspace is included in the Docker images built by ./Dockerfile and ./tests/integration/docker/Dockerfile.gm.

🔒 Suggested fix
       - uses: actions/checkout@v5
         with:
+          persist-credentials: false
           fetch-depth: 1

Apply the same change to all four checkout steps.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- uses: actions/checkout@v5
with:
fetch-depth: 0
fetch-depth: 1
- uses: actions/checkout@v5
with:
persist-credentials: false
fetch-depth: 1
🧰 Tools
🪛 zizmor (1.26.1)

[warning] 59-61: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)


[error] 59-59: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/migration_test.yml around lines 59 - 61, The workflow’s
checkout steps are leaving persisted git credentials enabled, which is
unnecessary for these jobs and increases leak risk. Update each of the four
actions/checkout usages in this workflow to disable credential persistence by
setting the appropriate checkout option in the existing with block, keeping
fetch-depth as-is. Use the actions/checkout steps in the migration test jobs as
the target for the change and apply it consistently to all four checkouts.

Source: Linters/SAST tools


- name: Log in to GHCR
uses: docker/login-action@v4
Expand Down Expand Up @@ -107,7 +107,7 @@ jobs:
steps:
- uses: actions/checkout@v5
with:
fetch-depth: 0
fetch-depth: 1

- name: Log in to GHCR
uses: docker/login-action@v4
Expand Down Expand Up @@ -152,7 +152,7 @@ jobs:
steps:
- uses: actions/checkout@v5
with:
fetch-depth: 0
fetch-depth: 1

- name: Set up Go
uses: actions/setup-go@v6
Expand Down Expand Up @@ -194,7 +194,7 @@ jobs:
steps:
- uses: actions/checkout@v5
with:
fetch-depth: 0
fetch-depth: 1

- name: Set up Go
uses: actions/setup-go@v6
Expand Down
110 changes: 71 additions & 39 deletions modules/network/genesis.go
Original file line number Diff line number Diff line change
@@ -1,48 +1,93 @@
package network

import (
"bytes"
"fmt"
"sort"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/bech32"

"github.com/evstack/ev-abci/modules/network/keeper"
"github.com/evstack/ev-abci/modules/network/types"
)

// InitGenesis initializes the network module's state from a provided genesis state.
func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState types.GenesisState) error {
// Set module params
if err := k.SetParams(ctx, genState.Params); err != nil {
return fmt.Errorf("set params: %s", err)
}

// Set validator indices
for _, vi := range genState.ValidatorIndices {
if err := k.SetValidatorIndex(ctx, vi.Address, uint16(vi.Index), vi.Power); err != nil {
return err
// Load attesters: validate pubkey/address match, then insert and assign indices.
attesters := make([]types.AttesterInfo, len(genState.AttesterInfos))
copy(attesters, genState.AttesterInfos)

for i := range attesters {
info := attesters[i]
pk, err := info.GetPubKey()
if err != nil {
return fmt.Errorf("attester %d: %w", i, err)
}
// Also add to attester set
if err := k.SetAttesterSetMember(ctx, vi.Address); err != nil {
return err
// Validate pubkey ↔ consensus_address match at the raw-bytes level so
// the check is independent of bech32 prefix (e.g. "cosmosvalcons" vs
// "celestiavalcons"). Whatever prefix was used in genesis, the 20-byte
// payload must equal the pubkey's Address().
_, rawAddr, decErr := bech32.DecodeAndConvert(info.ConsensusAddress)
if decErr != nil {
return fmt.Errorf("attester %d: decode consensus_address %q: %w", i, info.ConsensusAddress, decErr)
}
if !bytes.Equal(rawAddr, pk.Address()) {
return fmt.Errorf("attester %d: pubkey address mismatch (derived bytes %x, stated bytes %x)",
i, pk.Address(), rawAddr)
}
// Re-encode consensus_address with the runtime SDK config so the
// stored value matches what ConsAddress().String() produces elsewhere
// in the module at runtime.
derived := sdk.ConsAddress(pk.Address()).String()
info.ConsensusAddress = derived
attesters[i] = info
}
Comment on lines +25 to +49

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Reject duplicate attesters after consensus-address normalization.

After Line 46 rewrites the address to the runtime bech32 prefix, two genesis entries that decode to the same 20-byte consensus address will collapse onto the same keeper key. The later SetAttesterInfo / SetValidatorIndex writes then overwrite the earlier one instead of failing, which silently corrupts the fixed attester set.

🛡️ Suggested guard
 	attesters := make([]types.AttesterInfo, len(genState.AttesterInfos))
 	copy(attesters, genState.AttesterInfos)
+	seenConsensus := make(map[string]struct{}, len(attesters))

 	for i := range attesters {
 		info := attesters[i]
 		pk, err := info.GetPubKey()
 		if err != nil {
@@
 		// Re-encode consensus_address with the runtime SDK config so the
 		// stored value matches what ConsAddress().String() produces elsewhere
 		// in the module at runtime.
 		derived := sdk.ConsAddress(pk.Address()).String()
+		if _, exists := seenConsensus[derived]; exists {
+			return fmt.Errorf("attester %d: duplicate consensus_address %q", i, derived)
+		}
+		seenConsensus[derived] = struct{}{}
 		info.ConsensusAddress = derived
 		attesters[i] = info
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@modules/network/genesis.go` around lines 25 - 49, The loop over the attesters
slice must detect and reject duplicate consensus addresses after you normalize
the bech32 prefix: after computing derived :=
sdk.ConsAddress(pk.Address()).String() (in the attesters processing loop), keep
a map (e.g. map[string]int) keyed by the normalized derived string (or the raw
20-byte address pk.Address()) and if the derived address is already present
return an error indicating the duplicate attester (include both indices and the
conflicting ConsensusAddress) instead of allowing the later SetAttesterInfo /
SetValidatorIndex writes to silently overwrite the earlier entry; ensure you
reference the same symbols used in the loop (attesters, info.ConsensusAddress,
pk.Address(), derived) when implementing the check and error.


// Order by pubkey.Address() bytes ascending to match cmttypes.NewValidatorSet.
sort.Slice(attesters, func(i, j int) bool {
pki, _ := attesters[i].GetPubKey()
pkj, _ := attesters[j].GetPubKey()
return bytes.Compare(pki.Address(), pkj.Address()) < 0
})

for idx, info := range attesters {
if err := k.SetAttesterInfo(ctx, info.ConsensusAddress, &info); err != nil {
return fmt.Errorf("set attester info: %w", err)
}
if err := k.SetAttesterSetMember(ctx, info.ConsensusAddress); err != nil {
return fmt.Errorf("set attester set member: %w", err)
}
if err := k.SetValidatorIndex(ctx, info.ConsensusAddress, uint16(idx), 1); err != nil {
return fmt.Errorf("set validator index: %w", err)
Comment on lines +58 to +66

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Guard the uint16 validator-index cast.

Line 65 narrows idx to uint16 without checking the attester count first. A malformed genesis with too many attesters will wrap indices and make different attesters share the same bitmap position.

🚧 Suggested bound check
 import (
 	"bytes"
 	"fmt"
+	"math"
 	"sort"
@@
+	if len(attesters) > math.MaxUint16 {
+		return fmt.Errorf("too many attesters: %d", len(attesters))
+	}
+
 	for idx, info := range attesters {
 		if err := k.SetAttesterInfo(ctx, info.ConsensusAddress, &info); err != nil {
 			return fmt.Errorf("set attester info: %w", err)
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@modules/network/genesis.go` around lines 58 - 66, The loop over attesters
casts idx to uint16 when calling SetValidatorIndex, which can overflow if
len(attesters) > 65535; before calling SetValidatorIndex (in the for idx, info
:= range attesters loop) add a guard that checks if idx exceeds math.MaxUint16
(or 65535) and return a clear error (e.g., "too many attesters: exceeds uint16
limit") to prevent index wrapping; ensure this validation happens prior to
SetValidatorIndex invocation so SetValidatorIndex is only called with a safe
uint16 cast.

}
}

// Set attestation bitmaps
// Still load historical bitmaps if provided (upgrade/dump scenarios).
for _, ab := range genState.AttestationBitmaps {
if err := k.SetAttestationBitmap(ctx, ab.Height, ab.Bitmap); err != nil {
return err
}
// Store full attestation info using collections API
if err := k.StoredAttestationInfo.Set(ctx, ab.Height, ab); err != nil {
return err
}

if ab.SoftConfirmed {
if err := setSoftConfirmed(ctx, k, ab.Height); err != nil {
return err
}
}
}

// Legacy: genState.ValidatorIndices is now derived from AttesterInfos and
// ignored. Warn if non-empty so operators notice.
if len(genState.ValidatorIndices) > 0 {
k.Logger(ctx).Error("genesis.validator_indices is deprecated and ignored; use attester_infos")
}

return nil
}

Expand All @@ -51,54 +96,41 @@ func ExportGenesis(ctx sdk.Context, k keeper.Keeper) *types.GenesisState {
genesis := types.DefaultGenesisState()
genesis.Params = k.GetParams(ctx)

// Export validator indices using collections API
var validatorIndices []types.ValidatorIndex
// Iterate through all validator indices
if err := k.ValidatorIndex.Walk(ctx, nil, func(addr string, index uint16) (bool, error) {
power, err := k.GetValidatorPower(ctx, index)
if err != nil {
return false, fmt.Errorf("get validator power: %w", err)
}
validatorIndices = append(validatorIndices, types.ValidatorIndex{
Address: addr,
Index: uint32(index),
Power: power,
})
var attesters []types.AttesterInfo
if err := k.AttesterInfo.Walk(ctx, nil, func(_ string, info types.AttesterInfo) (bool, error) {
attesters = append(attesters, info)
return false, nil
}); err != nil {
panic(err)
}
genesis.ValidatorIndices = validatorIndices
sort.Slice(attesters, func(i, j int) bool {
pki, _ := attesters[i].GetPubKey()
pkj, _ := attesters[j].GetPubKey()
return bytes.Compare(pki.Address(), pkj.Address()) < 0
})
genesis.AttesterInfos = attesters

// Export attestation bitmaps using collections API
var attestationBitmaps []types.AttestationBitmap
// Iterate through all stored attestation info
if err := k.StoredAttestationInfo.Walk(ctx, nil, func(height int64, ab types.AttestationBitmap) (bool, error) {
if err := k.StoredAttestationInfo.Walk(ctx, nil, func(_ int64, ab types.AttestationBitmap) (bool, error) {
attestationBitmaps = append(attestationBitmaps, ab)
return false, nil
}); err != nil {
panic(err)
}
genesis.AttestationBitmaps = attestationBitmaps

// ValidatorIndices no longer exported: they are derived deterministically
// from AttesterInfos order.
genesis.ValidatorIndices = nil

return genesis
}

// Helper function to set soft confirmed status
func setSoftConfirmed(ctx sdk.Context, k keeper.Keeper, height int64) error {
// Get the existing attestation bitmap
ab, err := k.StoredAttestationInfo.Get(ctx, height)
if err != nil {
// If there's no existing attestation bitmap, we can't set it as soft confirmed
return err
}

// Set the SoftConfirmed field to true
ab.SoftConfirmed = true

// Update the attestation bitmap in the collection
if err := k.StoredAttestationInfo.Set(ctx, height, ab); err != nil {
return err
}
return nil
return k.StoredAttestationInfo.Set(ctx, height, ab)
}
5 changes: 2 additions & 3 deletions modules/network/keeper/abci.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,8 @@ func (k Keeper) processEpochEnd(ctx sdk.Context, epoch uint64) error {
return fmt.Errorf("pruning old data at epoch %d: %w", epoch, err)
}

if err := k.BuildValidatorIndexMap(ctx); err != nil {
return fmt.Errorf("rebuilding validator index map at epoch %d: %w", epoch, err)
}
// Validator indices are established at genesis and never mutate at runtime

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

comment not needed

// (MsgJoin/MsgLeave are disabled). Nothing to rebuild here.
return nil
}

Expand Down
Loading
Loading