From 02ca7fdcea7512720a21360413315be41b6d17de Mon Sep 17 00:00:00 2001 From: Vitalis Salis Date: Fri, 4 Oct 2024 16:38:07 +0300 Subject: [PATCH] chore: Enable gosec and do some internal types modifications (#16) Most of the errors from gosec stemmed from dangerous type conversions (e.g. from uint32 to uint16). Instead of doing checks everywhere inside the codebase on whether the type semantics are observed (e.g. checking that the uint32 is not larger than math.MaxUint16), I opted to handle this on the types definition and Babylon retrieval layer. I made gosec exclude the testutil/ and itest/ directories as they are related to tests. Also, for the following gosec errors I put a flag to remove them from the linter: - Two conversions from uint32 to uint16. Even though I performed the check previously, the static analyzer could not identify it. - Go file inclusion. This file is not passed from the web, but the user's machine, so it doesn't need further checks. --- .github/workflows/ci.yml | 9 +++++++-- .github/workflows/publish.yml | 9 +++++++-- Dockerfile | 2 +- clientcontroller/babylon.go | 29 +++++++++++++++++++++++++---- covenant/covenant.go | 19 +++++++++---------- covenant/covenant_test.go | 5 +++-- log/log.go | 3 ++- types/delegation.go | 7 ++++--- types/params.go | 8 ++++---- 9 files changed, 62 insertions(+), 29 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cd23d11..756881f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,14 +7,19 @@ on: jobs: lint_test: - uses: babylonlabs-io/.github/.github/workflows/reusable_go_lint_test.yml@v0.1.0 + uses: babylonlabs-io/.github/.github/workflows/reusable_go_lint_test.yml@v0.6.0 with: run-unit-tests: true run-integration-tests: true run-lint: true + run-build: true + run-gosec: true + gosec-args: "-exclude-generated -exclude-dir=itest -exclude-dir=testutil ./..." docker_pipeline: - uses: babylonlabs-io/.github/.github/workflows/reusable_docker_pipeline.yml@v0.1.0 + uses: babylonlabs-io/.github/.github/workflows/reusable_docker_pipeline.yml@v0.6.0 secrets: inherit with: publish: false + dockerfile: ./Dockerfile + repoName: covenant-emulator diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index d8e3d1b..74a64aa 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -9,15 +9,20 @@ on: jobs: lint_test: - uses: babylonlabs-io/.github/.github/workflows/reusable_go_lint_test.yml@v0.1.0 + uses: babylonlabs-io/.github/.github/workflows/reusable_go_lint_test.yml@v0.6.0 with: run-unit-tests: true run-integration-tests: true run-lint: true + run-build: true + run-gosec: true + gosec-args: "-exclude-generated -exclude-dir=itest -exclude-dir=testutil ./..." docker_pipeline: needs: ["lint_test"] - uses: babylonlabs-io/.github/.github/workflows/reusable_docker_pipeline.yml@v0.1.0 + uses: babylonlabs-io/.github/.github/workflows/reusable_docker_pipeline.yml@v0.6.0 secrets: inherit with: publish: true + dockerfile: ./Dockerfile + repoName: covenant-emulator diff --git a/Dockerfile b/Dockerfile index 31a4ca2..e8db5c2 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM golang:1.21.4 as builder +FROM golang:1.21.4 AS builder RUN apt-get update && apt-get install -y make git bash gcc curl jq diff --git a/clientcontroller/babylon.go b/clientcontroller/babylon.go index bdca5a6..121c1f4 100644 --- a/clientcontroller/babylon.go +++ b/clientcontroller/babylon.go @@ -3,6 +3,7 @@ package clientcontroller import ( "context" "fmt" + "math" "time" sdkmath "cosmossdk.io/math" @@ -112,6 +113,18 @@ func (bc *BabylonController) QueryStakingParamsByVersion(version uint32) (*types covenantPks = append(covenantPks, covPk) } + if stakingParamRes.Params.MinStakingTimeBlocks > math.MaxUint16 { + return nil, fmt.Errorf("babylon min staking time blocks (%d) is larger than the maximum uint16", stakingParamRes.Params.MinStakingTimeBlocks) + } + // #nosec G115 -- performed the conversion check above + minStakingTimeBlocksUint16 := uint16(stakingParamRes.Params.MinStakingTimeBlocks) + + if stakingParamRes.Params.MaxStakingTimeBlocks > math.MaxUint16 { + return nil, fmt.Errorf("babylon max staking time blocks (%d) is larger than the maximum uint16", stakingParamRes.Params.MaxStakingTimeBlocks) + } + // #nosec G115 -- performed the conversion check above + maxStakingTimeBlocksUint16 := uint16(stakingParamRes.Params.MaxStakingTimeBlocks) + return &types.StakingParams{ ComfirmationTimeBlocks: ckptParamRes.Params.BtcConfirmationDepth, FinalizationTimeoutBlocks: ckptParamRes.Params.CheckpointFinalizationTimeout, @@ -123,8 +136,8 @@ func (bc *BabylonController) QueryStakingParamsByVersion(version uint32) (*types MinComissionRate: stakingParamRes.Params.MinCommissionRate, MinUnbondingTime: stakingParamRes.Params.MinUnbondingTimeBlocks, UnbondingFee: btcutil.Amount(stakingParamRes.Params.UnbondingFeeSat), - MinStakingTime: uint16(stakingParamRes.Params.MinStakingTimeBlocks), - MaxStakingTime: uint16(stakingParamRes.Params.MaxStakingTimeBlocks), + MinStakingTime: minStakingTimeBlocksUint16, + MaxStakingTime: maxStakingTimeBlocksUint16, MinStakingValue: btcutil.Amount(stakingParamRes.Params.MinStakingValueSat), MaxStakingValue: btcutil.Amount(stakingParamRes.Params.MaxStakingValueSat), }, nil @@ -248,17 +261,25 @@ func DelegationRespToDelegation(del *btcstakingtypes.BTCDelegationResponse) (*ty fpBtcPks = append(fpBtcPks, fp.MustToBTCPK()) } + if del.UnbondingTime > uint32(math.MaxUint16) { + return nil, fmt.Errorf("unbonding time should be smaller than max uint16") + } + + if del.TotalSat > uint64(math.MaxInt64) { + return nil, fmt.Errorf("total sat (%d) is larger than the maximum int64", del.TotalSat) + } + return &types.Delegation{ BtcPk: del.BtcPk.MustToBTCPK(), FpBtcPks: fpBtcPks, - TotalSat: del.TotalSat, + TotalSat: btcutil.Amount(del.TotalSat), StartHeight: del.StartHeight, EndHeight: del.EndHeight, StakingTxHex: del.StakingTxHex, SlashingTxHex: del.SlashingTxHex, StakingOutputIdx: del.StakingOutputIdx, CovenantSigs: covenantSigs, - UnbondingTime: del.UnbondingTime, + UnbondingTime: uint16(del.UnbondingTime), BtcUndelegation: undelegation, }, nil } diff --git a/covenant/covenant.go b/covenant/covenant.go index 8edc20a..8fdfa51 100644 --- a/covenant/covenant.go +++ b/covenant/covenant.go @@ -152,7 +152,7 @@ func (ce *CovenantEmulator) AddCovenantSignatures(btcDels []*types.Delegation) ( if uint64(unbondingTime) <= minUnbondingTime { ce.logger.Error("invalid unbonding time", zap.Uint64("min_unbonding_time", minUnbondingTime), - zap.Uint32("got_unbonding_time", unbondingTime), + zap.Uint16("got_unbonding_time", unbondingTime), ) continue } @@ -171,11 +171,11 @@ func (ce *CovenantEmulator) AddCovenantSignatures(btcDels []*types.Delegation) ( continue } - if btcDel.TotalSat < uint64(params.MinStakingValue) || btcDel.TotalSat > uint64(params.MaxStakingValue) { + if btcDel.TotalSat < params.MinStakingValue || btcDel.TotalSat > params.MaxStakingValue { ce.logger.Error("invalid staking value", - zap.Uint64("min_staking_value", uint64(params.MinStakingValue)), - zap.Uint64("max_staking_value", uint64(params.MaxStakingValue)), - zap.Uint64("got_staking_value", btcDel.TotalSat), + zap.Int64("min_staking_value", int64(params.MinStakingValue)), + zap.Int64("max_staking_value", int64(params.MaxStakingValue)), + zap.Int64("got_staking_value", int64(btcDel.TotalSat)), ) continue } @@ -296,7 +296,7 @@ func signSlashUnbondingSignatures( del.FpBtcPks, params.CovenantPks, params.CovenantQuorum, - uint16(del.UnbondingTime), + del.UnbondingTime, btcutil.Amount(unbondingTx.TxOut[0].Value), btcNet, ) @@ -341,7 +341,6 @@ func signSlashAndUnbondSignatures( params *types.StakingParams, btcNet *chaincfg.Params, ) ([][]byte, *schnorr.Signature, error) { - // sign slash signatures with every finality providers stakingInfo, err := btcstaking.BuildStakingInfo( del.BtcPk, @@ -349,7 +348,7 @@ func signSlashAndUnbondSignatures( params.CovenantPks, params.CovenantQuorum, del.GetStakingTime(), - btcutil.Amount(del.TotalSat), + del.TotalSat, btcNet, ) if err != nil { @@ -429,7 +428,7 @@ func decodeDelegationTransactions(del *types.Delegation, params *types.StakingPa params.SlashingRate, params.SlashingPkScript, del.BtcPk, - uint16(del.UnbondingTime), + del.UnbondingTime, btcNet, ); err != nil { return nil, nil, fmt.Errorf("invalid txs in the delegation: %w", err) @@ -459,7 +458,7 @@ func decodeUndelegationTransactions(del *types.Delegation, params *types.Staking params.SlashingRate, params.SlashingPkScript, del.BtcPk, - uint16(del.UnbondingTime), + del.UnbondingTime, btcNet, ); err != nil { return nil, nil, fmt.Errorf("invalid txs in the undelegation: %w", err) diff --git a/covenant/covenant_test.go b/covenant/covenant_test.go index 2681eb4..5ed2891 100644 --- a/covenant/covenant_test.go +++ b/covenant/covenant_test.go @@ -2,6 +2,7 @@ package covenant_test import ( "encoding/hex" + "github.com/btcsuite/btcd/btcutil" "math/rand" "testing" @@ -87,8 +88,8 @@ func FuzzAddCovenantSig(f *testing.F) { FpBtcPks: fpPks, StartHeight: startHeight, // not relevant here EndHeight: startHeight + uint64(stakingTimeBlocks), - TotalSat: uint64(stakingValue), - UnbondingTime: uint32(unbondingTime), + TotalSat: btcutil.Amount(stakingValue), + UnbondingTime: unbondingTime, StakingTxHex: hex.EncodeToString(stakingTxBytes), StakingOutputIdx: stakingOutputIdx, SlashingTxHex: testInfo.SlashingTx.ToHexStr(), diff --git a/log/log.go b/log/log.go index e2f658a..7152b08 100644 --- a/log/log.go +++ b/log/log.go @@ -62,7 +62,8 @@ func NewRootLoggerWithFile(logFile string, level string) (*zap.Logger, error) { if err := util.MakeDirectory(filepath.Dir(logFile)); err != nil { return nil, err } - f, err := os.OpenFile(logFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0666) + // #nosec G304 - The log file path is provided by the user and not externally + f, err := os.OpenFile(logFile, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600) if err != nil { return nil, err } diff --git a/types/delegation.go b/types/delegation.go index ab557f4..92fa73c 100644 --- a/types/delegation.go +++ b/types/delegation.go @@ -1,6 +1,7 @@ package types import ( + "github.com/btcsuite/btcd/btcutil" "math" bbn "github.com/babylonlabs-io/babylon/types" @@ -23,7 +24,7 @@ type Delegation struct { EndHeight uint64 // The total amount of BTC stakes in this delegation // quantified in satoshi - TotalSat uint64 + TotalSat btcutil.Amount // The hex string of the staking tx StakingTxHex string // The index of the staking output in the staking tx @@ -32,7 +33,7 @@ type Delegation struct { SlashingTxHex string // UnbondingTime describes how long the funds will be locked either in unbonding output // or slashing change output - UnbondingTime uint32 + UnbondingTime uint16 // The signatures on the slashing tx // by the covenants (i.e., SKs corresponding to covenant_pks in params) // It will be a part of the witness for the staking tx output. @@ -47,7 +48,7 @@ type Delegation struct { // HasCovenantQuorum returns whether a delegation has sufficient sigs // from Covenant members to make a quorum func (d *Delegation) HasCovenantQuorum(quorum uint32) bool { - return uint32(len(d.CovenantSigs)) >= quorum && d.BtcUndelegation.HasAllSignatures(quorum) + return len(d.CovenantSigs) >= int(quorum) && d.BtcUndelegation.HasAllSignatures(quorum) } func (d *Delegation) GetStakingTime() uint16 { diff --git a/types/params.go b/types/params.go index 0974e3a..252c4c7 100644 --- a/types/params.go +++ b/types/params.go @@ -36,16 +36,16 @@ type StakingParams struct { // Fee required by unbonding transaction UnbondingFee btcutil.Amount - // Minimum staking time required by bayblon + // Minimum staking time required by babylon MinStakingTime uint16 - // Maximum staking time required by bayblon + // Maximum staking time required by babylon MaxStakingTime uint16 - // Minimum staking value required by bayblon + // Minimum staking value required by babylon MinStakingValue btcutil.Amount - // Maximum staking value required by bayblon + // Maximum staking value required by babylon MaxStakingValue btcutil.Amount }