From 84ffbcfb7a2912475fe6a58087d7921d14a79f91 Mon Sep 17 00:00:00 2001 From: Rongjian Lan Date: Tue, 10 Mar 2020 18:04:17 -0700 Subject: [PATCH] [audit] Various fixes and todos for auditing; add log for collect rewards (#2448) * More logs and checks before processing staking txns * fix import * Refactor block proposal * Various fixes and todos for auditing; add log for collect rewards * Fix lint * fix comment --- consensus/leader.go | 1 + consensus/threshold.go | 1 + consensus/validator.go | 3 ++- consensus/view_change.go | 1 + core/blockchain.go | 1 + core/staking_verifier.go | 30 +++++++++++++++++------ core/state_processor.go | 18 ++++++++------ core/state_transition.go | 42 ++++++++++++++++++++------------ core/types/block.go | 11 ++++++++- internal/chain/engine.go | 7 ++++++ node/node_explorer.go | 1 + node/node_handler.go | 12 ++++----- node/node_newblock.go | 2 +- node/worker/worker.go | 1 + staking/params.go | 6 +++-- staking/types/delegation_test.go | 2 +- staking/types/validator.go | 7 +++--- 17 files changed, 99 insertions(+), 47 deletions(-) diff --git a/consensus/leader.go b/consensus/leader.go index 9054f7b82..a704ef8b2 100644 --- a/consensus/leader.go +++ b/consensus/leader.go @@ -311,6 +311,7 @@ func (consensus *Consensus) onCommit(msg *msg_pb.Message) { return } + // TODO(audit): verify signature on hash+blockNum+viewID (add a hard fork) blockNumHash := make([]byte, 8) binary.LittleEndian.PutUint64(blockNumHash, recvMsg.BlockNum) commitPayload := append(blockNumHash, recvMsg.BlockHash[:]...) diff --git a/consensus/threshold.go b/consensus/threshold.go index 135f45831..be4a7f266 100644 --- a/consensus/threshold.go +++ b/consensus/threshold.go @@ -37,6 +37,7 @@ func (consensus *Consensus) didReachPrepareQuorum() error { consensus.aggregatedPrepareSig = aggSig consensus.FBFTLog.AddMessage(FBFTMsg) // Leader add commit phase signature + // TODO(audit): sign signature on hash+blockNum+viewID (add a hard fork) blockNumHash := [8]byte{} binary.LittleEndian.PutUint64(blockNumHash[:], consensus.blockNum) commitPayload := append(blockNumHash[:], consensus.blockHash[:]...) diff --git a/consensus/validator.go b/consensus/validator.go index d47107988..26b4f9f7b 100644 --- a/consensus/validator.go +++ b/consensus/validator.go @@ -196,7 +196,7 @@ func (consensus *Consensus) onPrepared(msg *msg_pb.Message) { groupID := []nodeconfig.GroupID{nodeconfig.NewGroupIDByShardID(nodeconfig.ShardID(consensus.ShardID))} for i, key := range consensus.PubKey.PublicKey { networkMessage, _ := consensus.construct( - // TODO: should only sign on block hash + // TODO(audit): sign signature on hash+blockNum+viewID (add a hard fork) msg_pb.MessageType_COMMIT, append(blockNumBytes, consensus.blockHash[:]...), key, consensus.priKey.PrivateKey[i], @@ -249,6 +249,7 @@ func (consensus *Consensus) onCommitted(msg *msg_pb.Message) { return } + // TODO(audit): verify signature on hash+blockNum+viewID (add a hard fork) blockNumBytes := make([]byte, 8) binary.LittleEndian.PutUint64(blockNumBytes, recvMsg.BlockNum) commitPayload := append(blockNumBytes, recvMsg.BlockHash[:]...) diff --git a/consensus/view_change.go b/consensus/view_change.go index 135f24ef5..6366f6066 100644 --- a/consensus/view_change.go +++ b/consensus/view_change.go @@ -377,6 +377,7 @@ func (consensus *Consensus) onViewChange(msg *msg_pb.Message) { consensus.aggregatedPrepareSig = aggSig consensus.prepareBitmap = mask // Leader sign and add commit message + // TODO(audit): verify signature on hash+blockNum+viewID (add a hard fork) blockNumBytes := [8]byte{} binary.LittleEndian.PutUint64(blockNumBytes[:], consensus.blockNum) commitPayload := append(blockNumBytes[:], consensus.blockHash[:]...) diff --git a/core/blockchain.go b/core/blockchain.go index ce43a9906..74c70b404 100644 --- a/core/blockchain.go +++ b/core/blockchain.go @@ -265,6 +265,7 @@ func (bc *BlockChain) ValidateNewBlock(block *types.Block) error { return err } + // Verify all the hash roots (state, txns, receipts, cross-shard) if err := bc.Validator().ValidateState( block, state, receipts, cxReceipts, usedGas, ); err != nil { diff --git a/core/staking_verifier.go b/core/staking_verifier.go index b774024bb..5fa74413a 100644 --- a/core/staking_verifier.go +++ b/core/staking_verifier.go @@ -4,11 +4,14 @@ import ( "bytes" "math/big" + "github.com/harmony-one/harmony/internal/utils" + + "github.com/pkg/errors" + "github.com/ethereum/go-ethereum/common" "github.com/harmony-one/harmony/core/vm" common2 "github.com/harmony-one/harmony/internal/common" staking "github.com/harmony-one/harmony/staking/types" - "github.com/pkg/errors" ) var ( @@ -98,10 +101,9 @@ func VerifyAndEditValidatorFromMsg( return nil, errCommissionRateChangeTooHigh } - // TODO: make sure we are reading from the correct snapshot snapshotValidator, err := chainContext.ReadValidatorSnapshot(wrapper.Address) if err != nil { - return nil, err + return nil, errors.WithMessage(err, "Validator snapshot not found.") } rateAtBeginningOfEpoch := snapshotValidator.Validator.Rate @@ -172,9 +174,14 @@ func VerifyAndDelegateFromMsg( ); err != nil { return nil, nil, err } - // Return remaining balance to be deducted for delegation if delegateBalance.Cmp(big.NewInt(0)) < 0 { - return nil, nil, errInsufficientBalanceForStake // shouldn't really happen + return nil, nil, errNegativeAmount // shouldn't really happen + } + // Return remaining balance to be deducted for delegation + if !CanTransfer(stateDB, msg.DelegatorAddress, delegateBalance) { + return nil, nil, errors.Wrapf( + errInsufficientBalanceForStake, "had %v, tried to stake %v", + stateDB.GetBalance(msg.DelegatorAddress), delegateBalance) } return wrapper, delegateBalance, nil } @@ -189,7 +196,9 @@ func VerifyAndDelegateFromMsg( } // If no redelegation, create new delegation if !CanTransfer(stateDB, msg.DelegatorAddress, msg.Amount) { - return nil, nil, errInsufficientBalanceForStake + return nil, nil, errors.Wrapf( + errInsufficientBalanceForStake, "had %v, tried to stake %v", + stateDB.GetBalance(msg.DelegatorAddress), msg.Amount) } wrapper.Delegations = append( wrapper.Delegations, staking.NewDelegation( @@ -270,8 +279,15 @@ func VerifyAndCollectRewardsFromDelegation( delegation := &wrapper.Delegations[delegation.Index] if delegation.Reward.Cmp(common.Big0) > 0 { totalRewards.Add(totalRewards, delegation.Reward) + delegation.Reward.SetUint64(0) } - delegation.Reward.SetUint64(0) + } else { + utils.Logger().Warn(). + Str("validator", delegation.ValidatorAddress.String()). + Uint64("delegation index", delegation.Index). + Int("delegations length", len(wrapper.Delegations)). + Msg("Delegation index out of bound") + return nil, nil, errors.New("Delegation index out of bound") } if err := wrapper.SanityCheck( staking.DoNotEnforceMaxBLS, diff --git a/core/state_processor.go b/core/state_processor.go index 4bae53d63..851c9af93 100644 --- a/core/state_processor.go +++ b/core/state_processor.go @@ -99,7 +99,7 @@ func (p *StateProcessor) Process( allLogs = append(allLogs, receipt.Logs...) } - // Iterate over staking transactions + // Iterate over and process the staking transactions L := len(block.Transactions()) for i, tx := range block.StakingTransactions() { statedb.Prepare(tx.Hash(), block.Hash(), i+L) @@ -119,14 +119,14 @@ func (p *StateProcessor) Process( err := ApplyIncomingReceipt(p.config, statedb, header, cx) if err != nil { return nil, nil, - nil, 0, nil, ctxerror.New("cannot apply incoming receipts").WithCause(err) + nil, 0, nil, ctxerror.New("[Process] Cannot apply incoming receipts").WithCause(err) } } slashes := slash.Records{} if s := header.Slashes(); len(s) > 0 { if err := rlp.DecodeBytes(s, &slashes); err != nil { - return nil, nil, nil, 0, nil, ctxerror.New("cannot finalize block").WithCause(err) + return nil, nil, nil, 0, nil, ctxerror.New("[Process] Cannot finalize block").WithCause(err) } } @@ -136,7 +136,7 @@ func (p *StateProcessor) Process( receipts, outcxs, incxs, block.StakingTransactions(), slashes, ) if err != nil { - return nil, nil, nil, 0, nil, ctxerror.New("cannot finalize block").WithCause(err) + return nil, nil, nil, 0, nil, ctxerror.New("[Process] Cannot finalize block").WithCause(err) } return receipts, outcxs, allLogs, *usedGas, payout, nil @@ -253,11 +253,8 @@ func ApplyStakingTransaction( // Apply the transaction to the current state (included in the env) gas, err = ApplyStakingMessage(vmenv, msg, gp, bc) - utils.Logger().Info().Msgf("ApplyStakingMessage: usedGas: %v, err: %v, stakingTxn:", gas, err) - - // even there is error, we charge it if err != nil { - return nil, gas, err + return nil, 0, err } // Update the state with pending changes @@ -272,6 +269,11 @@ func ApplyStakingTransaction( receipt.TxHash = tx.Hash() receipt.GasUsed = gas + // TODO(audit): add more log to staking txns; expose them in block explorer. + if config.IsReceiptLog(header.Epoch()) { + receipt.Logs = statedb.GetLogs(tx.Hash()) + } + return receipt, gas, nil } diff --git a/core/state_transition.go b/core/state_transition.go index b495c4fb6..f946ac9d9 100644 --- a/core/state_transition.go +++ b/core/state_transition.go @@ -20,6 +20,9 @@ import ( "math" "math/big" + staking2 "github.com/harmony-one/harmony/staking" + "github.com/harmony-one/harmony/staking/network" + "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/rlp" "github.com/harmony-one/harmony/core/types" @@ -291,7 +294,6 @@ func (st *StateTransition) StakingTransitionDb() (usedGas uint64, err error) { homestead := st.evm.ChainConfig().IsS3(st.evm.EpochNumber) // s3 includes homestead // Pay intrinsic gas - // TODO: propose staking-specific formula for staking transaction gas, err := IntrinsicGas(st.data, false, homestead, msg.Type() == types.StakeCreateVal) if err != nil { @@ -354,7 +356,15 @@ func (st *StateTransition) StakingTransitionDb() (usedGas uint64, err error) { if msg.From() != stkMsg.DelegatorAddress { return 0, errInvalidSigner } - err = st.verifyAndApplyCollectRewards(stkMsg) + collectedRewards, err := st.verifyAndApplyCollectRewards(stkMsg) + if err == nil { + st.state.AddLog(&types.Log{ + Address: stkMsg.DelegatorAddress, + Topics: []common.Hash{staking2.CollectRewardsTopic}, + Data: collectedRewards.Bytes(), + BlockNumber: st.evm.BlockNumber.Uint64(), + }) + } default: return 0, staking.ErrInvalidStakingKind } @@ -373,10 +383,10 @@ func (st *StateTransition) verifyAndApplyCreateValidatorTx( if err != nil { return err } - if err := st.state.UpdateValidatorWrapper(wrapper.Validator.Address, wrapper); err != nil { + if err := st.state.UpdateValidatorWrapper(wrapper.Address, wrapper); err != nil { return err } - st.state.SetValidatorFlag(wrapper.Validator.Address) + st.state.SetValidatorFlag(wrapper.Address) st.state.SubBalance(wrapper.Address, createValidator.Amount) return nil } @@ -396,11 +406,10 @@ func (st *StateTransition) verifyAndApplyDelegateTx(delegate *staking.Delegate) if err != nil { return err } - if err := st.state.UpdateValidatorWrapper(wrapper.Validator.Address, wrapper); err != nil { - return err - } + st.state.SubBalance(delegate.DelegatorAddress, balanceToBeDeducted) - return nil + + return st.state.UpdateValidatorWrapper(wrapper.Address, wrapper) } func (st *StateTransition) verifyAndApplyUndelegateTx(undelegate *staking.Undelegate) error { @@ -408,28 +417,29 @@ func (st *StateTransition) verifyAndApplyUndelegateTx(undelegate *staking.Undele if err != nil { return err } - return st.state.UpdateValidatorWrapper(wrapper.Validator.Address, wrapper) + return st.state.UpdateValidatorWrapper(wrapper.Address, wrapper) } -func (st *StateTransition) verifyAndApplyCollectRewards(collectRewards *staking.CollectRewards) error { +func (st *StateTransition) verifyAndApplyCollectRewards(collectRewards *staking.CollectRewards) (*big.Int, error) { if st.bc == nil { - return errors.New("[CollectRewards] No chain context provided") + return network.NoReward, errors.New("[CollectRewards] No chain context provided") } + // TODO(audit): make sure the delegation index is always consistent with onchain data delegations, err := st.bc.ReadDelegationsByDelegator(collectRewards.DelegatorAddress) if err != nil { - return err + return network.NoReward, err } updatedValidatorWrappers, totalRewards, err := VerifyAndCollectRewardsFromDelegation( st.state, delegations, ) if err != nil { - return err + return network.NoReward, err } for _, wrapper := range updatedValidatorWrappers { - if err := st.state.UpdateValidatorWrapper(wrapper.Validator.Address, wrapper); err != nil { - return err + if err := st.state.UpdateValidatorWrapper(wrapper.Address, wrapper); err != nil { + return network.NoReward, err } } st.state.AddBalance(collectRewards.DelegatorAddress, totalRewards) - return nil + return totalRewards, nil } diff --git a/core/types/block.go b/core/types/block.go index c194c62d8..6ee5fe167 100644 --- a/core/types/block.go +++ b/core/types/block.go @@ -294,12 +294,19 @@ func NewBlock( header *block.Header, txs []*Transaction, receipts []*Receipt, outcxs []*CXReceipt, incxs []*CXReceiptsProof, stks []*staking.StakingTransaction) *Block { + b := &Block{header: CopyHeader(header)} if len(receipts) != len(txs)+len(stks) { + utils.Logger().Error(). + Int("receiptsLen", len(receipts)). + Int("txnsLen", len(txs)). + Int("stakingTxnsLen", len(stks)). + Msg("Length of receipts doesn't match length of transactions") return nil } + // Put transactions into block if len(txs) == 0 && len(stks) == 0 { b.header.SetTxHash(EmptyRootHash) } else { @@ -315,6 +322,7 @@ func NewBlock( )) } + // Put receipts into block if len(receipts) == 0 { b.header.SetReceiptHash(EmptyRootHash) } else { @@ -322,8 +330,8 @@ func NewBlock( b.header.SetBloom(CreateBloom(receipts)) } + // Put cross-shard receipts (ingres/egress) into block b.header.SetOutgoingReceiptHash(CXReceipts(outcxs).ComputeMerkleRoot()) - if len(incxs) == 0 { b.header.SetIncomingReceiptHash(EmptyRootHash) } else { @@ -332,6 +340,7 @@ func NewBlock( copy(b.incomingReceipts, incxs) } + // Great! Block is finally finalized. return b } diff --git a/internal/chain/engine.go b/internal/chain/engine.go index 472ed97b9..f1b84eb60 100644 --- a/internal/chain/engine.go +++ b/internal/chain/engine.go @@ -208,6 +208,8 @@ func (e *engineImpl) VerifySeal(chain engine.ChainReader, header *block.Header) if err != nil { return errors.Wrapf(err, "cannot decoded shard state") } + + // TODO(audit): reuse a singleton decider and not recreate it for every single block d := quorum.NewDecider(quorum.SuperMajorityStake) d.SetMyPublicKeyProvider(func() (*multibls.PublicKey, error) { return nil, nil @@ -236,6 +238,7 @@ func (e *engineImpl) VerifySeal(chain engine.ChainReader, header *block.Header) } } + // TODO(audit): verify signature on hash+blockNum+viewID (add a hard fork) blockNumHash := make([]byte, 8) binary.LittleEndian.PutUint64(blockNumHash, header.Number().Uint64()-1) lastCommitPayload := append(blockNumHash, parentHash[:]...) @@ -289,6 +292,7 @@ func (e *engineImpl) Finalize( } } + // Finalize the state root header.SetRoot(state.IntermediateRoot(chain.Config().IsS3(header.Epoch()))) return types.NewBlock(header, txs, receipts, outcxs, incxs, stks), payout, nil } @@ -445,6 +449,8 @@ func (e *engineImpl) VerifyHeaderWithSignature(chain engine.ChainReader, header if err != nil { return errors.Wrapf(err, "cannot read shard state") } + + // TODO(audit): reuse a singleton decider and not recreate it for every single block d := quorum.NewDecider(quorum.SuperMajorityStake) d.SetMyPublicKeyProvider(func() (*multibls.PublicKey, error) { return nil, nil @@ -470,6 +476,7 @@ func (e *engineImpl) VerifyHeaderWithSignature(chain engine.ChainReader, header "need", quorumCount, "got", count) } } + // TODO(audit): verify signature on hash+blockNum+viewID (add a hard fork) blockNumHash := make([]byte, 8) binary.LittleEndian.PutUint64(blockNumHash, header.Number().Uint64()) commitPayload := append(blockNumHash, hash[:]...) diff --git a/node/node_explorer.go b/node/node_explorer.go index 5e4d483c2..c54eae19d 100644 --- a/node/node_explorer.go +++ b/node/node_explorer.go @@ -52,6 +52,7 @@ func (node *Node) ExplorerMessageHandler(payload []byte) { return } + // TODO(audit): verify signature on hash+blockNum+viewID (add a hard fork) blockNumHash := make([]byte, 8) binary.LittleEndian.PutUint64(blockNumHash, recvMsg.BlockNum) commitPayload := append(blockNumHash, recvMsg.BlockHash[:]...) diff --git a/node/node_handler.go b/node/node_handler.go index f3ef46253..85570b73a 100644 --- a/node/node_handler.go +++ b/node/node_handler.go @@ -337,9 +337,9 @@ func (node *Node) VerifyNewBlock(newBlock *types.Block) error { utils.Logger().Error(). Str("blockHash", newBlock.Hash().Hex()). Err(err). - Msg("cannot ValidateHeader for the new block") + Msg("[VerifyNewBlock] Cannot validate header for the new block") return ctxerror.New( - "cannot ValidateHeader for the new block", + "[VerifyNewBlock] Cannot validate header for the new block", "blockHash", newBlock.Hash(), ).WithCause(err) @@ -349,8 +349,8 @@ func (node *Node) VerifyNewBlock(newBlock *types.Block) error { utils.Logger().Error(). Uint32("my shard ID", node.Blockchain().ShardID()). Uint32("new block's shard ID", newBlock.ShardID()). - Msg("wrong shard ID") - return ctxerror.New("wrong shard ID", + Msg("[VerifyNewBlock] Wrong shard ID of the new block") + return ctxerror.New("[VerifyNewBlock] Wrong shard ID of the new block", "my shard ID", node.Blockchain().ShardID(), "new block's shard ID", newBlock.ShardID(), ) @@ -362,9 +362,9 @@ func (node *Node) VerifyNewBlock(newBlock *types.Block) error { utils.Logger().Error(). Str("blockHash", newBlock.Hash().Hex()). Err(err). - Msg("cannot VerifyShardState for the new block") + Msg("[VerifyNewBlock] Cannot verify shard state for the new block") return ctxerror.New( - "cannot VerifyShardState for the new block", "blockHash", + "[VerifyNewBlock] Cannot verify shard state for the new block", "blockHash", newBlock.Hash(), ).WithCause(err) } diff --git a/node/node_newblock.go b/node/node_newblock.go index 13eea4238..d3eb12b86 100644 --- a/node/node_newblock.go +++ b/node/node_newblock.go @@ -1,6 +1,7 @@ package node import ( + "errors" "sort" "strings" "time" @@ -11,7 +12,6 @@ import ( "github.com/harmony-one/harmony/internal/utils" "github.com/harmony-one/harmony/shard" staking "github.com/harmony-one/harmony/staking/types" - "github.com/pkg/errors" ) // Constants of proposing a new block diff --git a/node/worker/worker.go b/node/worker/worker.go index d3fc58325..0538f870d 100644 --- a/node/worker/worker.go +++ b/node/worker/worker.go @@ -335,6 +335,7 @@ func (w *Worker) CollectVerifiedSlashes() error { if d := pending; len(d) > 0 { pending, failures = w.verifySlashes(d) } + if f := failures; len(f) > 0 { if err := w.chain.DeleteFromPendingSlashingCandidates(f); err != nil { return err diff --git a/staking/params.go b/staking/params.go index 8df37a9fd..f9bb8f5d5 100644 --- a/staking/params.go +++ b/staking/params.go @@ -7,10 +7,12 @@ import ( const ( isValidatorKeyStr = "Harmony/IsValidator/Key/v1" isValidatorStr = "Harmony/IsValidator/Value/v1" + collectRewardsStr = "Harmony/CollectRewards" ) // keys used to retrieve staking related informatio var ( - IsValidatorKey = crypto.Keccak256Hash([]byte(isValidatorKeyStr)) - IsValidator = crypto.Keccak256Hash([]byte(isValidatorStr)) + IsValidatorKey = crypto.Keccak256Hash([]byte(isValidatorKeyStr)) + IsValidator = crypto.Keccak256Hash([]byte(isValidatorStr)) + CollectRewardsTopic = crypto.Keccak256Hash([]byte(collectRewardsStr)) ) diff --git a/staking/types/delegation_test.go b/staking/types/delegation_test.go index d6111f4e7..6e00f0c90 100644 --- a/staking/types/delegation_test.go +++ b/staking/types/delegation_test.go @@ -41,7 +41,7 @@ func TestUndelegate(t *testing.T) { } func TestTotalInUndelegation(t *testing.T) { - var totalAmount *big.Int = delegation.TotalInUndelegation() + var totalAmount = delegation.TotalInUndelegation() // check the total amount of undelegation if totalAmount.Cmp(big.NewInt(3000)) != 0 { diff --git a/staking/types/validator.go b/staking/types/validator.go index 0b2106fb4..35be044e6 100644 --- a/staking/types/validator.go +++ b/staking/types/validator.go @@ -180,6 +180,7 @@ func (v *Validator) SanityCheck(oneThirdExtrn int) error { return err } + // TODO(audit): add limit on the number of bls keys one can have. if len(v.SlotPubKeys) == 0 { return errNeedAtLeastOneSlotKey } @@ -200,10 +201,8 @@ func (v *Validator) SanityCheck(oneThirdExtrn int) error { return errNilMaxTotalDelegation } - // if I'm not banned, then I must - // ensure that MinSelfDelegation >= 1 ONE - if v.EPOSStatus != effective.Banned && - v.MinSelfDelegation.Cmp(big.NewInt(denominations.One)) < 0 { + // MinSelfDelegation must be >= 1 ONE + if v.MinSelfDelegation.Cmp(big.NewInt(denominations.One)) < 0 { return errors.Wrapf( errMinSelfDelegationTooSmall, "delegation-given %s", v.MinSelfDelegation.String(),