Skip to content

Security/Fix: Resolve critical security vulnerabilities and core module edge cases #4609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Mayne-X wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Security/Fix: Resolve critical security vulnerabilities and core module edge cases #4609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
73 changes: 48 additions & 25 deletions pkg/crypto/aes256_gcm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,48 @@ import (
io "io"

"github.com/pkg/errors"
"golang.org/x/crypto/pbkdf2"
)

// EncryptAES256GCMBase64 encrypts the given string plaintext using AES-256-GCM with the given password and returns the base64-encoded ciphertext.
const (
// pbkdf2Iterations is the number of PBKDF2 iterations used for key derivation.
// OWASP 2023 recommends 600000 for PBKDF2-HMAC-SHA256.
pbkdf2Iterations = 600000
// pbkdf2KeyLen is the AES-256 key length in bytes.
pbkdf2KeyLen = 32
)

// getAESKey derives a 32-byte AES key from the given password using PBKDF2.
// This replaces the previous SHA-256-based derivation which was vulnerable to
// offline brute-force attacks.
func getAESKey(password string) []byte {
return getAESKeyPBKDF2(password)
}

// getAESKeyPBKDF2 derives a key using PBKDF2-HMAC-SHA256 with a static salt.
func getAESKeyPBKDF2(password string) []byte {
salt := []byte("zeta-chain-aes-256-gcm")
return pbkdf2.Key([]byte(password), salt, pbkdf2Iterations, pbkdf2KeyLen, sha256.New)
Comment on lines +30 to +33

@coderabbitai coderabbitai Bot Jun 27, 2026

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 | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo '--- file ---'
wc -l pkg/crypto/aes256_gcm.go
echo '--- excerpt ---'
cat -n pkg/crypto/aes256_gcm.go | sed -n '1,220p'
echo '--- search call sites ---'
rg -n "EncryptAES|DecryptAES|getAESKeyPBKDF2|pbkdf2Iterations|pbkdf2KeyLen" . -S

Repository: zeta-chain/node

Length of output: 8851

Use a per-ciphertext salt for PBKDF2.

The fixed salt makes every password derive the same AES key, so the KDF no longer provides per-message uniqueness. Store a random salt with each ciphertext and include version/KDF metadata so legacy payloads can still be decrypted. Also defer PBKDF2 until after the ciphertext length check to avoid unnecessary work on obviously invalid input.

🤖 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 `@pkg/crypto/aes256_gcm.go` around lines 30 - 33, The PBKDF2 key derivation in
getAESKeyPBKDF2 currently uses a fixed salt, so update the AES-GCM flow to
generate and persist a random per-ciphertext salt along with version/KDF
metadata and use that metadata during decryption for backward-compatible
handling of legacy payloads. Adjust the encrypt/decrypt paths in aes256_gcm.go
so the salt is read from or written into the ciphertext format, and move the
PBKDF2 call to happen only after the ciphertext length check so invalid inputs
are rejected before any derivation work.

}
Comment on lines +31 to +34

@greptile-apps greptile-apps Bot Jun 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 security Static PBKDF2 salt breaks per-credential isolation

A salt's primary job in PBKDF2 is to ensure that two users with identical passwords still derive different keys, and to make precomputed (rainbow-table) attacks against the specific application salt economically infeasible. Using the hardcoded constant "zeta-chain-aes-256-gcm" means every encrypted relayer key in every deployment shares the same key-derivation context. An attacker who breaks one node's password gets a derivation target that applies to all deployments — the 600 k iterations slow them down but the single-salt surface remains fixed. The salt should be generated fresh per-encryption (e.g., crypto/rand, 16+ bytes) and prepended to the ciphertext alongside the nonce so Decrypt can recover it.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: pkg/crypto/aes256_gcm.go
Line: 31-34

Comment:
**Static PBKDF2 salt breaks per-credential isolation**

A salt's primary job in PBKDF2 is to ensure that two users with identical passwords still derive different keys, and to make precomputed (rainbow-table) attacks against the specific application salt economically infeasible. Using the hardcoded constant `"zeta-chain-aes-256-gcm"` means every encrypted relayer key in every deployment shares the same key-derivation context. An attacker who breaks one node's password gets a derivation target that applies to all deployments — the 600 k iterations slow them down but the single-salt surface remains fixed. The salt should be generated fresh per-encryption (e.g., `crypto/rand`, 16+ bytes) and prepended to the ciphertext alongside the nonce so `Decrypt` can recover it.

How can I resolve this? If you propose a fix, please make it concise.


// getAESKeyLegacy derives a key using a single SHA-256 round (old insecure method).
// Retained for backward compatibility with encrypted data from before the PBKDF2 migration.
func getAESKeyLegacy(key string) []byte {
h := sha256.New()
h.Write([]byte(key))
return h.Sum(nil)
}

// EncryptAES256GCMBase64 encrypts the given string plaintext using AES-256-GCM with the given password
// and returns the base64-encoded ciphertext.
func EncryptAES256GCMBase64(plaintext string, password string) (string, error) {
// validate the input
if plaintext == "" {
return "", errors.New("plaintext must not be empty")
}
if password == "" {
return "", errors.New("password must not be empty")
}

// encrypt the plaintext
ciphertext, err := EncryptAES256GCM([]byte(plaintext), password)
if err != nil {
return "", errors.Wrap(err, "failed to encrypt string plaintext")
Expand All @@ -31,21 +60,18 @@ func EncryptAES256GCMBase64(plaintext string, password string) (string, error) {

// DecryptAES256GCMBase64 decrypts the given base64-encoded ciphertext using AES-256-GCM with the given password.
func DecryptAES256GCMBase64(ciphertextBase64 string, password string) (string, error) {
// validate the input
if ciphertextBase64 == "" {
return "", errors.New("ciphertext must not be empty")
}
if password == "" {
return "", errors.New("password must not be empty")
}

// decode the base64-encoded ciphertext
ciphertext, err := base64.StdEncoding.DecodeString(ciphertextBase64)
if err != nil {
return "", errors.Wrap(err, "failed to decode base64 ciphertext")
}

// decrypt the ciphertext
plaintext, err := DecryptAES256GCM(ciphertext, password)
if err != nil {
return "", errors.Wrap(err, "failed to decrypt ciphertext")
Expand All @@ -55,54 +81,59 @@ func DecryptAES256GCMBase64(ciphertextBase64 string, password string) (string, e

// EncryptAES256GCM encrypts the given plaintext using AES-256-GCM with the given password.
func EncryptAES256GCM(plaintext []byte, password string) ([]byte, error) {
// create AES cipher
block, err := aes.NewCipher(getAESKey(password))
if err != nil {
return nil, err
}

// create GCM mode
gcm, err := cipher.NewGCM(block)
if err != nil {
return nil, err
}

// generate random nonce
nonce := make([]byte, gcm.NonceSize())
if _, err := io.ReadFull(rand.Reader, nonce); err != nil {
return nil, err
}

// encrypt the plaintext
ciphertext := gcm.Seal(nonce, nonce, plaintext, nil)

return ciphertext, nil
}

// DecryptAES256GCM decrypts the given ciphertext using AES-256-GCM with the given password.
// It first tries PBKDF2 key derivation, then falls back to legacy SHA-256 for backward compatibility.
func DecryptAES256GCM(ciphertext []byte, password string) ([]byte, error) {
// create AES cipher
block, err := aes.NewCipher(getAESKey(password))
// try PBKDF2 key derivation first
plaintext, err := decryptWithKey(ciphertext, getAESKeyPBKDF2(password))
if err != nil {
// fall back to legacy SHA-256 key derivation
plaintext, err = decryptWithKey(ciphertext, getAESKeyLegacy(password))
if err != nil {
return nil, errors.Wrap(err, "failed to decrypt with both PBKDF2 and legacy key derivation")
}
}
return plaintext, nil
}

// decryptWithKey decrypts ciphertext with the given AES key.
func decryptWithKey(ciphertext []byte, key []byte) ([]byte, error) {
block, err := aes.NewCipher(key)
if err != nil {
return nil, err
}

// create GCM mode
gcm, err := cipher.NewGCM(block)
if err != nil {
return nil, err
}

// get the nonce size
nonceSize := gcm.NonceSize()
if len(ciphertext) < nonceSize {
return nil, errors.New("ciphertext too short")
}

// extract the nonce from the ciphertext
nonce, ciphertext := ciphertext[:nonceSize], ciphertext[nonceSize:]

// decrypt the ciphertext
// #nosec G407 false positive https://github.com/securego/gosec/issues/1211
plaintext, err := gcm.Open(nil, nonce, ciphertext, nil)
if err != nil {
Expand All @@ -111,11 +142,3 @@ func DecryptAES256GCM(ciphertext []byte, password string) ([]byte, error) {

return plaintext, nil
}

// getAESKey uses SHA-256 to create a 32-byte key for AES encryption.
func getAESKey(key string) []byte {
h := sha256.New()
h.Write([]byte(key))

return h.Sum(nil)
}
10 changes: 5 additions & 5 deletions x/crosschain/types/cctx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,27 +222,27 @@ func (m *CrossChainTx) UpdateCurrentOutbound(
}

// SetAbort sets the CCTX status to Aborted with the given error message.
func (m CrossChainTx) SetAbort(messages StatusMessages) {
func (m *CrossChainTx) SetAbort(messages StatusMessages) {
m.CctxStatus.UpdateStatusAndErrorMessages(CctxStatus_Aborted, messages)
}

// SetPendingRevert sets the CCTX status to PendingRevert with the given error message.
func (m CrossChainTx) SetPendingRevert(messages StatusMessages) {
func (m *CrossChainTx) SetPendingRevert(messages StatusMessages) {
m.CctxStatus.UpdateStatusAndErrorMessages(CctxStatus_PendingRevert, messages)
}

// SetPendingOutbound sets the CCTX status to PendingOutbound with the given error message.
func (m CrossChainTx) SetPendingOutbound(messages StatusMessages) {
func (m *CrossChainTx) SetPendingOutbound(messages StatusMessages) {
m.CctxStatus.UpdateStatusAndErrorMessages(CctxStatus_PendingOutbound, messages)
}

// SetOutboundMined sets the CCTX status to OutboundMined with the given error message.
func (m CrossChainTx) SetOutboundMined() {
func (m *CrossChainTx) SetOutboundMined() {
m.CctxStatus.UpdateStatusAndErrorMessages(CctxStatus_OutboundMined, StatusMessages{})
}

// SetReverted sets the CCTX status to Reverted with the given error message.
func (m CrossChainTx) SetReverted() {
func (m *CrossChainTx) SetReverted() {
m.CctxStatus.UpdateStatusAndErrorMessages(CctxStatus_Reverted, StatusMessages{})
}

Expand Down
39 changes: 18 additions & 21 deletions x/crosschain/types/inbound_params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package types

import (
"fmt"

"github.com/pkg/errors"
)

func (m InboundParams) Validate() error {
Expand All @@ -13,26 +15,21 @@ func (m InboundParams) Validate() error {
return fmt.Errorf("amount cannot be nil")
}

// Disabled checks
// TODO: Improve the checks, move the validation call to a new place and reenable
// https://github.com/zeta-chain/node/issues/2234
// https://github.com/zeta-chain/node/issues/2235
//if err := ValidateAddressForChain(m.Sender, m.SenderChainId) err != nil {
// return err
//}
//if m.TxOrigin != "" {
// errTxOrigin := ValidateAddressForChain(m.TxOrigin, m.SenderChainId)
// if errTxOrigin != nil {
// return errTxOrigin
// }
//}
//if err := ValidateHashForChain(m.ObservedHash, m.SenderChainId); err != nil {
// return errors.Wrap(err, "invalid inbound tx observed hash")
//}
//if m.BallotIndex != "" {
// if err := ValidateCCTXIndex(m.BallotIndex); err != nil {
// return errors.Wrap(err, "invalid inbound tx ballot index")
// }
//}
if err := ValidateAddressForChain(m.Sender, m.SenderChainId); err != nil {
return err
}
if m.TxOrigin != "" {
if err := ValidateAddressForChain(m.TxOrigin, m.SenderChainId); err != nil {
return err
}
}
if err := ValidateHashForChain(m.ObservedHash, m.SenderChainId); err != nil {
return errors.Wrap(err, "invalid inbound tx observed hash")
Comment on lines +26 to +27

@coderabbitai coderabbitai Bot Jun 27, 2026

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.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Tighten Ethereum/Zeta hash validation to a fixed hash length.

Line 26 now relies on ValidateHashForChain, but x/crosschain/types/validate.go:26-35 currently accepts any hex-decodable Ethereum/Zeta value. Inputs like 0x01 would pass here even though they are not transaction hashes, so this hardening still leaves malformed hashes admissible. Please enforce a 32-byte decoded length in the shared helper before relying on it from InboundParams.Validate() (this will also cover the outbound path).

🤖 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 `@x/crosschain/types/inbound_params.go` around lines 26 - 27, The shared hash
check used by InboundParams.Validate() still accepts any hex-decodable
Ethereum/Zeta value, so tighten ValidateHashForChain in validate.go to require a
32-byte decoded hash before returning success. Update the helper to reject
shorter/longer inputs like 0x01, and keep the existing callers such as
InboundParams.Validate() and the outbound path relying on that centralized
validation.

}
if m.BallotIndex != "" {
if err := ValidateCCTXIndex(m.BallotIndex); err != nil {
return errors.Wrap(err, "invalid inbound tx ballot index")
}
}
return nil
}
31 changes: 13 additions & 18 deletions x/crosschain/types/outbound_params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,24 +39,19 @@ func (m OutboundParams) Validate() error {
return fmt.Errorf("amount cannot be nil")
}

// Disabled checks
// TODO: Improve the checks, move the validation call to a new place and reenable
// https://github.com/zeta-chain/node/issues/2234
// https://github.com/zeta-chain/node/issues/2235
//if err := ValidateAddressForChain(m.Receiver, m.ReceiverChainId); err != nil {
// return err
//}
//if m.BallotIndex != "" {
//
// if err := ValidateCCTXIndex(m.BallotIndex); err != nil {
// return errors.Wrap(err, "invalid outbound tx ballot index")
// }
//}
//if m.Hash != "" {
// if err := ValidateHashForChain(m.Hash, m.ReceiverChainId); err != nil {
// return errors.Wrap(err, "invalid outbound tx hash")
// }
//}
if err := ValidateAddressForChain(m.Receiver, m.ReceiverChainId); err != nil {
return err
}
if m.BallotIndex != "" {
if err := ValidateCCTXIndex(m.BallotIndex); err != nil {
return errors.Wrap(err, "invalid outbound tx ballot index")
}
}
if m.Hash != "" {
if err := ValidateHashForChain(m.Hash, m.ReceiverChainId); err != nil {
return errors.Wrap(err, "invalid outbound tx hash")
}
}

return nil
}
7 changes: 6 additions & 1 deletion x/fungible/keeper/evm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,12 @@ func (k Keeper) DeployZRC20Contract(
newCoin.ForeignChainId = chain.ChainId
newCoin.GasLimit = gasLimit.Uint64()
if liquidityCap == nil {
liquidityCap = ptr.Ptr(sdkmath.NewUint(types.DefaultLiquidityCap).MulUint64(uint64(newCoin.Decimals)))
// compute 10^decimals for the liquidity cap
tenToDecimals := sdkmath.NewUint(1)
for i := uint32(0); i < newCoin.Decimals; i++ {
tenToDecimals = tenToDecimals.MulUint64(10)
}
liquidityCap = ptr.Ptr(sdkmath.NewUint(types.DefaultLiquidityCap).Mul(tenToDecimals))
}
newCoin.LiquidityCap = *liquidityCap
k.SetForeignCoins(ctx, newCoin)
Expand Down
8 changes: 6 additions & 2 deletions x/fungible/keeper/gas_coin_and_pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,12 @@ func (k Keeper) SetupChainGasCoinAndPool(
return ethcommon.Address{}, err
}
amount := big.NewInt(10)
// #nosec G115 always in range
amount.Exp(amount, big.NewInt(int64(decimals-1)), nil)
if decimals == 0 {
amount = big.NewInt(1)
} else {
// #nosec G115 always in range
amount.Exp(amount, big.NewInt(int64(decimals-1)), nil)
}
amountAZeta := big.NewInt(1e17)

_, err = k.DepositZRC20(ctx, zrc20Addr, types.ModuleAddressEVM, amount)
Expand Down
6 changes: 4 additions & 2 deletions x/fungible/keeper/msg_server_deploy_fungible_coin_zrc20.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,11 @@ func (k msgServer) DeployFungibleCoinZRC20(
return nil, cosmoserrors.Wrap(authoritytypes.ErrUnauthorized, err.Error())
}

tmpCtx, commit := ctx.CacheContext()
if msg.CoinType == coin.CoinType_Gas {
// #nosec G115 always in range
address, err = k.SetupChainGasCoinAndPool(
ctx,
tmpCtx,
msg.ForeignChainId,
msg.Name,
msg.Symbol,
Expand All @@ -66,7 +67,7 @@ func (k msgServer) DeployFungibleCoinZRC20(
} else {
// #nosec G115 always in range
address, err = k.DeployZRC20Contract(
ctx,
tmpCtx,
msg.Name,
msg.Symbol,
uint8(msg.Decimals),
Expand Down Expand Up @@ -98,6 +99,7 @@ func (k msgServer) DeployFungibleCoinZRC20(
if err != nil {
return nil, cosmoserrors.Wrapf(err, "failed to emit event")
}
commit()

return &types.MsgDeployFungibleCoinZRC20Response{
Address: address.Hex(),
Expand Down
12 changes: 7 additions & 5 deletions x/fungible/keeper/msg_server_deploy_system_contract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,31 +25,32 @@ func (k msgServer) DeploySystemContracts(
}

// uniswap v2 factory
factory, err := k.DeployUniswapV2Factory(ctx)
tmpCtx, commit := ctx.CacheContext()
factory, err := k.DeployUniswapV2Factory(tmpCtx)
if err != nil {
return nil, cosmoserrors.Wrapf(err, "failed to deploy UniswapV2Factory")
}

// wzeta contract
wzeta, err := k.DeployWZETA(ctx)
wzeta, err := k.DeployWZETA(tmpCtx)
if err != nil {
return nil, cosmoserrors.Wrapf(err, "failed to DeployWZetaContract")
}

// uniswap v2 router
router, err := k.DeployUniswapV2Router02(ctx, factory, wzeta)
router, err := k.DeployUniswapV2Router02(tmpCtx, factory, wzeta)
if err != nil {
return nil, cosmoserrors.Wrapf(err, "failed to deploy UniswapV2Router02")
}

// connector zevm
connector, err := k.DeployConnectorZEVM(ctx, wzeta)
connector, err := k.DeployConnectorZEVM(tmpCtx, wzeta)
if err != nil {
return nil, cosmoserrors.Wrapf(err, "failed to deploy ConnectorZEVM")
}

// system contract
systemContract, err := k.DeploySystemContract(ctx, wzeta, factory, router)
systemContract, err := k.DeploySystemContract(tmpCtx, wzeta, factory, router)
if err != nil {
return nil, cosmoserrors.Wrapf(err, "failed to deploy SystemContract")
}
Expand All @@ -72,6 +73,7 @@ func (k msgServer) DeploySystemContracts(
)
return nil, cosmoserrors.Wrapf(types.ErrEmitEvent, "failed to emit event (%s)", err.Error())
}
commit()

return &types.MsgDeploySystemContractsResponse{
UniswapV2Factory: factory.Hex(),
Expand Down
Loading
Loading