diff --git a/actpool/actpool.go b/actpool/actpool.go index baa79b595f..dd08294d7d 100644 --- a/actpool/actpool.go +++ b/actpool/actpool.go @@ -309,44 +309,50 @@ func (ap *actPool) GetActionByHash(hash hash.Hash32B) (*iproto.ActionPb, error) func (ap *actPool) validateTsf(tsf *action.Transfer) error { // Reject coinbase transfer if tsf.IsCoinbase { - logger.Error().Msg("Error when validating transfer") + logger.Error().Msg("Error when validating whether transfer is coinbase") return errors.Wrapf(ErrTransfer, "coinbase transfer") } // Reject oversized transfer if tsf.TotalSize() > TransferSizeLimit { - logger.Error().Msg("Error when validating transfer") + logger.Error().Msg("Error when validating transfer's data size") return errors.Wrapf(ErrActPool, "oversized data") } // Reject transfer of negative amount if tsf.Amount.Sign() < 0 { - logger.Error().Msg("Error when validating transfer") + logger.Error().Msg("Error when validating transfer's amount") return errors.Wrapf(ErrBalance, "negative value") } + // check if sender's address is valid - _, err := iotxaddress.GetPubkeyHash(tsf.Sender) - if err != nil { - return errors.Wrap(err, "error when getting the pubkey hash") + if _, err := iotxaddress.GetPubkeyHash(tsf.Sender); err != nil { + logger.Error().Msg("Error when validating transfer sender's address") + return errors.Wrapf(err, "error when validating sender's address %s", tsf.Sender) + } + // check if recipient's address is valid + if _, err := iotxaddress.GetPubkeyHash(tsf.Recipient); err != nil { + logger.Error().Msg("Error when validating transfer recipient's address") + return errors.Wrapf(err, "error when validating recipient's address %s", tsf.Recipient) } sender, err := iotxaddress.GetAddressByPubkey(iotxaddress.IsTestnet, iotxaddress.ChainID, tsf.SenderPublicKey) if err != nil { - logger.Error().Err(err).Msg("Error when validating transfer") + logger.Error().Err(err).Msg("Error when validating transfer sender's public key") return errors.Wrapf(err, "invalid address") } // Verify transfer using sender's public key if err := tsf.Verify(sender); err != nil { - logger.Error().Err(err).Msg("Error when validating transfer") + logger.Error().Err(err).Msg("Error when validating transfer's signature") return errors.Wrapf(err, "failed to verify Transfer signature") } // Reject transfer if nonce is too low confirmedNonce, err := ap.bc.Nonce(tsf.Sender) if err != nil { - logger.Error().Err(err).Msg("Error when validating transfer") + logger.Error().Err(err).Msg("Error when validating transfer's nonce") return errors.Wrapf(err, "invalid nonce value") } pendingNonce := confirmedNonce + 1 if pendingNonce > tsf.Nonce { - logger.Error().Msg("Error when validating transfer") + logger.Error().Msg("Error when validating transfer's nonce") return errors.Wrapf(ErrNonce, "nonce too low") } return nil @@ -365,33 +371,42 @@ func (ap *actPool) validateExecution(exec *action.Execution) error { } // Reject execution of negative amount if exec.Amount.Sign() < 0 { - logger.Error().Msg("Error when validating execution amount") + logger.Error().Msg("Error when validating execution's amount") return errors.Wrapf(ErrBalance, "negative value") } + // check if executor's address is valid if _, err := iotxaddress.GetPubkeyHash(exec.Executor); err != nil { - return errors.Wrap(err, "error when getting the pubkey hash") + logger.Error().Msg("Error when validating executor's address") + return errors.Wrapf(err, "error when validating executor's address %s", exec.Executor) + } + // check if contract's address is valid + if exec.Contract != action.EmptyAddress { + if _, err := iotxaddress.GetPubkeyHash(exec.Contract); err != nil { + logger.Error().Msg("Error when validating contract's address") + return errors.Wrapf(err, "error when validating contract's address %s", exec.Contract) + } } executor, err := iotxaddress.GetAddressByPubkey(iotxaddress.IsTestnet, iotxaddress.ChainID, exec.ExecutorPubKey) if err != nil { - logger.Error().Err(err).Msg("Error when validating execution") + logger.Error().Err(err).Msg("Error when validating executor's public key") return errors.Wrapf(err, "invalid address") } // Verify transfer using executor's public key if err := exec.Verify(executor); err != nil { - logger.Error().Err(err).Msg("Error when validating execution") + logger.Error().Err(err).Msg("Error when validating execution's signature") return errors.Wrapf(err, "failed to verify Execution signature") } // Reject transfer if nonce is too low confirmedNonce, err := ap.bc.Nonce(executor.RawAddress) if err != nil { - logger.Error().Err(err).Msg("Error when validating execution") + logger.Error().Err(err).Msg("Error when validating execution's nonce") return errors.Wrapf(err, "invalid nonce value") } pendingNonce := confirmedNonce + 1 if pendingNonce > exec.Nonce { - logger.Error().Msg("Error when validating execution") + logger.Error().Msg("Error when validating execution's nonce") return errors.Wrapf(ErrNonce, "nonce too low") } return nil @@ -401,57 +416,66 @@ func (ap *actPool) validateExecution(exec *action.Execution) error { func (ap *actPool) validateVote(vote *action.Vote) error { // Reject oversized vote if vote.TotalSize() > VoteSizeLimit { - logger.Error().Msg("Error when validating vote") + logger.Error().Msg("Error when validating vote's data size") return errors.Wrapf(ErrActPool, "oversized data") } selfPublicKey, err := vote.SelfPublicKey() if err != nil { - return err + logger.Error().Err(err).Msg("Error when validating voter's public key") + return errors.Wrapf(err, "failed to get voter's public key") + } + + // check if voter's address is valid + if _, err := iotxaddress.GetPubkeyHash(vote.GetVote().VoterAddress); err != nil { + logger.Error().Err(err).Msg("Error when validating voter's address") + return errors.Wrapf(err, "error when validating voter's address %s", vote.GetVote().VoterAddress) + } + // check if votee's address is valid + if vote.GetVote().VoteeAddress != action.EmptyAddress { + if _, err := iotxaddress.GetPubkeyHash(vote.GetVote().VoteeAddress); err != nil { + logger.Error().Err(err).Msg("Error when validating votee's address") + return errors.Wrapf(err, "error when validating votee's address %s", vote.GetVote().VoteeAddress) + } } + voter, err := iotxaddress.GetAddressByPubkey(iotxaddress.IsTestnet, iotxaddress.ChainID, selfPublicKey) if err != nil { - logger.Error().Err(err).Msg("Error when validating vote") - return errors.Wrapf(err, "invalid voter address") + logger.Error().Err(err).Msg("Error when validating voter's public key") + return errors.Wrapf(err, "invalid address") } // Verify vote using voter's public key if err := vote.Verify(voter); err != nil { - logger.Error().Err(err).Msg("Error when validating vote") + logger.Error().Err(err).Msg("Error when validating vote's signature") return errors.Wrapf(err, "failed to verify Vote signature") } // Reject vote if nonce is too low confirmedNonce, err := ap.bc.Nonce(voter.RawAddress) if err != nil { - logger.Error().Err(err).Msg("Error when validating vote") + logger.Error().Err(err).Msg("Error when validating vote's nonce") return errors.Wrapf(err, "invalid nonce value") } pbVote := vote.GetVote() if pbVote.VoteeAddress != "" { // Reject vote if votee is not a candidate - if err != nil { - logger.Error(). - Err(err).Hex("voter", pbVote.SelfPubkey[:]).Str("votee", pbVote.VoteeAddress). - Msg("Error when validating vote") - return errors.Wrapf(err, "invalid votee public key: %s", pbVote.VoteeAddress) - } voteeState, err := ap.bc.StateByAddr(pbVote.VoteeAddress) if err != nil { logger.Error().Err(err). Hex("voter", pbVote.SelfPubkey[:]).Str("votee", pbVote.VoteeAddress). - Msg("Error when validating vote") + Msg("Error when validating votee's state") return errors.Wrapf(err, "cannot find votee's state: %s", pbVote.VoteeAddress) } if voter.RawAddress != pbVote.VoteeAddress && !voteeState.IsCandidate { logger.Error().Err(ErrVotee). Hex("voter", pbVote.SelfPubkey[:]).Str("votee", pbVote.VoteeAddress). - Msg("Error when validating vote") + Msg("Error when validating votee's state") return errors.Wrapf(ErrVotee, "votee has not self-nominated: %s", pbVote.VoteeAddress) } } pendingNonce := confirmedNonce + 1 if pendingNonce > vote.Nonce { - logger.Error().Msg("Error when validating vote") + logger.Error().Msg("Error when validating vote's nonce") return errors.Wrapf(ErrNonce, "nonce too low") } return nil diff --git a/actpool/actpool_test.go b/actpool/actpool_test.go index 3a5d9a2583..557fb1e673 100644 --- a/actpool/actpool_test.go +++ b/actpool/actpool_test.go @@ -9,6 +9,7 @@ package actpool import ( "fmt" "math/big" + "strings" "testing" "github.com/golang/mock/gomock" @@ -61,26 +62,31 @@ func TestActPool_validateTsf(t *testing.T) { require.Nil(err) ap, ok := Ap.(*actPool) require.True(ok) - // Case I: Coinbase Transfer + // Case I: Coinbase transfer coinbaseTsf := action.Transfer{IsCoinbase: true} err = ap.validateTsf(&coinbaseTsf) require.Equal(ErrTransfer, errors.Cause(err)) - // Case II: Oversized Data + // Case II: Oversized data tmpPayload := [32769]byte{} payload := tmpPayload[:] tsf := action.Transfer{Payload: payload} err = ap.validateTsf(&tsf) require.Equal(ErrActPool, errors.Cause(err)) - // Case III: Negative Amount + // Case III: Negative amount tsf = action.Transfer{Amount: big.NewInt(-100)} err = ap.validateTsf(&tsf) - require.NotNil(ErrBalance, errors.Cause(err)) - // Case IV: Signature Verification Fails + require.Equal(ErrBalance, errors.Cause(err)) + // Case IV: Invalid address + tsf = action.Transfer{Sender: addr1.RawAddress, Recipient: "io1qyqsyqcyq5narhapakcsrhksfajfcpl24us3xp38zwvsep", Amount: big.NewInt(1)} + err = ap.validateTsf(&tsf) + require.Error(err) + require.True(strings.Contains(err.Error(), "error when validating recipient's address")) + // Case V: Signature verification fails unsignedTsf, err := action.NewTransfer(uint64(1), big.NewInt(1), addr1.RawAddress, addr1.RawAddress) require.NoError(err) err = ap.validateTsf(unsignedTsf) require.Equal(action.ErrTransferError, errors.Cause(err)) - // Case V: Nonce is too low + // Case VI: Nonce is too low prevTsf, _ := signedTransfer(addr1, addr1, uint64(1), big.NewInt(50)) err = ap.AddTsf(prevTsf) require.NoError(err) @@ -105,7 +111,7 @@ func TestActPool_validateVote(t *testing.T) { require.Nil(err) ap, ok := Ap.(*actPool) require.True(ok) - // Case I: Oversized Data + // Case I: Oversized data tmpSelfPubKey := [32769]byte{} selfPubKey := tmpSelfPubKey[:] vote := action.Vote{ @@ -118,13 +124,39 @@ func TestActPool_validateVote(t *testing.T) { } err = ap.validateVote(&vote) require.Equal(ErrActPool, errors.Cause(err)) - // Case II: Signature Verification Fails + // Case II: Invalid voter's public key + vote = action.Vote{ + ActionPb: &iproto.ActionPb{ + Action: &iproto.ActionPb_Vote{ + Vote: &iproto.VotePb{}, + }, + }, + } + err = ap.validateVote(&vote) + require.Error(err) + require.True(strings.Contains(err.Error(), "failed to get voter's public key")) + // Case III: Invalid address + vote = action.Vote{ + ActionPb: &iproto.ActionPb{ + Action: &iproto.ActionPb_Vote{ + Vote: &iproto.VotePb{ + SelfPubkey: addr1.PublicKey[:], + VoterAddress: addr1.RawAddress, + VoteeAddress: "123", + }, + }, + }, + } + err = ap.validateVote(&vote) + require.Error(err) + require.True(strings.Contains(err.Error(), "error when validating votee's address")) + // Case IV: Signature verification fails unsignedVote, err := action.NewVote(1, addr1.RawAddress, addr2.RawAddress) unsignedVote.GetVote().SelfPubkey = addr1.PublicKey[:] require.NoError(err) err = ap.validateVote(unsignedVote) require.Equal(action.ErrVoteError, errors.Cause(err)) - // Case III: Nonce is too low + // Case V: Nonce is too low prevTsf, _ := signedTransfer(addr1, addr1, uint64(1), big.NewInt(50)) err = ap.AddTsf(prevTsf) require.NoError(err) @@ -134,7 +166,7 @@ func TestActPool_validateVote(t *testing.T) { nVote, _ := signedVote(addr1, addr1, uint64(1)) err = ap.validateVote(nVote) require.Equal(ErrNonce, errors.Cause(err)) - // Case IV: Votee is not a candidate + // Case VI: Votee is not a candidate vote2, _ := signedVote(addr1, addr2, uint64(2)) err = ap.validateVote(vote2) require.Equal(ErrVotee, errors.Cause(err)) diff --git a/actpool/actQueue.go b/actpool/actqueue.go similarity index 100% rename from actpool/actQueue.go rename to actpool/actqueue.go diff --git a/actpool/actQueue_test.go b/actpool/actqueue_test.go similarity index 100% rename from actpool/actQueue_test.go rename to actpool/actqueue_test.go diff --git a/blockchain/block_test.go b/blockchain/block_test.go index 1467397019..655d736749 100644 --- a/blockchain/block_test.go +++ b/blockchain/block_test.go @@ -367,7 +367,7 @@ func TestWrongCoinbaseTsf(t *testing.T) { err = val.Validate(blk, 2, hash) require.Error(err) require.True( - strings.Contains(err.Error(), "Wrong number of coinbase transfers"), + strings.Contains(err.Error(), "wrong number of coinbase transfers"), ) // extra coinbase transfer @@ -377,7 +377,7 @@ func TestWrongCoinbaseTsf(t *testing.T) { err = val.Validate(blk, 2, hash) require.Error(err) require.True( - strings.Contains(err.Error(), "Wrong number of coinbase transfers"), + strings.Contains(err.Error(), "wrong number of coinbase transfers"), ) // no transfer @@ -387,6 +387,33 @@ func TestWrongCoinbaseTsf(t *testing.T) { err = val.Validate(blk, 2, hash) require.Error(err) require.True( - strings.Contains(err.Error(), "Wrong number of coinbase transfers"), + strings.Contains(err.Error(), "wrong number of coinbase transfers"), ) } + +func TestWrongAddress(t *testing.T) { + val := validator{} + invalidRecipient := "io1qyqsyqcyq5narhapakcsrhksfajfcpl24us3xp38zwvsep" + tsf, err := action.NewTransfer(1, big.NewInt(1), ta.Addrinfo["producer"].RawAddress, invalidRecipient) + require.NoError(t, err) + blk1 := NewBlock(1, 3, hash.ZeroHash32B, clock.New(), []*action.Transfer{tsf}, nil, nil) + err = val.verifyActions(blk1) + require.Error(t, err) + require.True(t, strings.Contains(err.Error(), "failed to validate transfer recipient's address")) + + invalidVotee := "ioaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + vote, err := action.NewVote(1, ta.Addrinfo["producer"].RawAddress, invalidVotee) + require.NoError(t, err) + blk2 := NewBlock(1, 3, hash.ZeroHash32B, clock.New(), nil, []*action.Vote{vote}, nil) + err = val.verifyActions(blk2) + require.Error(t, err) + require.True(t, strings.Contains(err.Error(), "failed to validate votee's address")) + + invalidContract := "123" + execution, err := action.NewExecution(ta.Addrinfo["producer"].RawAddress, invalidContract, 1, big.NewInt(1), uint64(100000), uint64(10), []byte{}) + require.NoError(t, err) + blk3 := NewBlock(1, 3, hash.ZeroHash32B, clock.New(), nil, nil, []*action.Execution{execution}) + err = val.verifyActions(blk3) + require.Error(t, err) + require.True(t, strings.Contains(err.Error(), "failed to validate contract's address")) +} diff --git a/blockchain/blockchain_test.go b/blockchain/blockchain_test.go index f2517611ce..74525ae5e5 100644 --- a/blockchain/blockchain_test.go +++ b/blockchain/blockchain_test.go @@ -436,7 +436,7 @@ func TestBlockchain_MintNewDummyBlock(t *testing.T) { err = val.Validate(blk, 0, tipHash) require.Error(err) require.True( - strings.Contains(err.Error(), "Fail to verify block's signature"), + strings.Contains(err.Error(), "failed to verify block's signature"), ) } diff --git a/blockchain/blockvalidator.go b/blockchain/blockvalidator.go index b158100974..68a360c673 100644 --- a/blockchain/blockvalidator.go +++ b/blockchain/blockvalidator.go @@ -55,7 +55,7 @@ func (v *validator) Validate(blk *Block, tipHeight uint64, tipHash hash.Hash32B) if blk.Header.height != 0 && blk.Header.height != tipHeight+1 { return errors.Wrapf( ErrInvalidTipHeight, - "Wrong block height %d, expecting %d", + "wrong block height %d, expecting %d", blk.Header.height, tipHeight+1) } @@ -63,7 +63,7 @@ func (v *validator) Validate(blk *Block, tipHeight uint64, tipHash hash.Hash32B) if blk.Header.prevBlockHash != tipHash { return errors.Wrapf( ErrInvalidBlock, - "Wrong prev hash %x, expecting %x", + "wrong prev hash %x, expecting %x", blk.Header.prevBlockHash, tipHash) } @@ -78,7 +78,7 @@ func (v *validator) Validate(blk *Block, tipHeight uint64, tipHash hash.Hash32B) if !crypto.EC283.Verify(blk.Header.Pubkey, blkHash[:], blk.Header.blockSig) { return errors.Wrapf( ErrInvalidBlock, - "Fail to verify block's signature with public key: %x", + "failed to verify block's signature with public key: %x", blk.Header.Pubkey, tipHash) } @@ -89,7 +89,7 @@ func (v *validator) Validate(blk *Block, tipHeight uint64, tipHash hash.Hash32B) if bytes.Compare(hashExpect[:], hashActual[:]) != 0 { return errors.Wrapf( ErrInvalidBlock, - "Wrong tx hash %x, expecting %x", + "wrong tx hash %x, expecting %x", hashActual, hashActual) } @@ -102,7 +102,7 @@ func (v *validator) Validate(blk *Block, tipHeight uint64, tipHash hash.Hash32B) } func (v *validator) verifyActions(blk *Block) error { - // Verify transfers and votes (balance is checked in CommitStateChanges) + // Verify transfers, votes, and executions (balance is checked in CommitStateChanges) confirmedNonceMap := make(map[string]uint64) accountNonceMap := make(map[string][]uint64) var wg sync.WaitGroup @@ -110,16 +110,26 @@ func (v *validator) verifyActions(blk *Block) error { var correctAction uint64 var coinbaseCount uint64 for _, tsf := range blk.Transfers { - if blk.Header.height > 0 && !tsf.IsCoinbase { - // Verify Nonce - // Verify Signature - // Verify Coinbase transfer + // Verify Address + // Verify Nonce + // Verify Signature + // Verify Coinbase transfer + if !tsf.IsCoinbase { + if _, err := iotxaddress.GetPubkeyHash(tsf.Sender); err != nil { + return errors.Wrapf(err, "failed to validate transfer sender's address %s", tsf.Sender) + } + if _, err := iotxaddress.GetPubkeyHash(tsf.Recipient); err != nil { + return errors.Wrapf(err, "failed to validate transfer recipient's address %s", tsf.Recipient) + } + } + + if blk.Header.height > 0 && !tsf.IsCoinbase { // Store the nonce of the sender and verify later if _, ok := confirmedNonceMap[tsf.Sender]; !ok { accountNonce, err := v.sf.Nonce(tsf.Sender) if err != nil { - return errors.Wrap(err, "Failed to get the nonce of transfer sender") + return errors.Wrap(err, "failed to get the nonce of transfer sender") } confirmedNonceMap[tsf.Sender] = accountNonce accountNonceMap[tsf.Sender] = make([]uint64, 0) @@ -162,15 +172,26 @@ func (v *validator) verifyActions(blk *Block) error { }(tsf, &correctAction, &coinbaseCount) } for _, vote := range blk.Votes { + // Verify Address // Verify Nonce // Verify Signature + + if _, err := iotxaddress.GetPubkeyHash(vote.GetVote().VoterAddress); err != nil { + return errors.Wrapf(err, "failed to validate voter's address %s", vote.GetVote().VoterAddress) + } + if vote.GetVote().VoteeAddress != action.EmptyAddress { + if _, err := iotxaddress.GetPubkeyHash(vote.GetVote().VoteeAddress); err != nil { + return errors.Wrapf(err, "failed to validate votee's address %s", vote.GetVote().VoteeAddress) + } + } + if blk.Header.height > 0 { // Store the nonce of the voter and verify later voterAddress := vote.GetVote().VoterAddress if _, ok := confirmedNonceMap[voterAddress]; !ok { accountNonce, err := v.sf.Nonce(voterAddress) if err != nil { - return errors.Wrap(err, "Failed to get the nonce of the voter") + return errors.Wrap(err, "failed to get the nonce of the voter") } confirmedNonceMap[voterAddress] = accountNonce accountNonceMap[voterAddress] = make([]uint64, 0) @@ -196,17 +217,28 @@ func (v *validator) verifyActions(blk *Block) error { }(vote, &correctAction) } for _, execution := range blk.Executions { + // Verify Address // Verify Nonce // Verify Signature // Verify Gas // Verify Amount + + if _, err := iotxaddress.GetPubkeyHash(execution.Executor); err != nil { + return errors.Wrapf(err, "failed to validate executor's address %s", execution.Executor) + } + if execution.Contract != action.EmptyAddress { + if _, err := iotxaddress.GetPubkeyHash(execution.Contract); err != nil { + return errors.Wrapf(err, "failed to validate contract's address %s", execution.Contract) + } + } + if blk.Header.height > 0 { // Store the nonce of the executor and verify later executor := execution.Executor if _, ok := confirmedNonceMap[executor]; !ok { accountNonce, err := v.sf.Nonce(executor) if err != nil { - return errors.Wrap(err, "Failed to get the nonce of the executor") + return errors.Wrap(err, "failed to get the nonce of the executor") } confirmedNonceMap[executor] = accountNonce accountNonceMap[executor] = make([]uint64, 0) @@ -247,12 +279,12 @@ func (v *validator) verifyActions(blk *Block) error { if (blk.Header.height != 0 && coinbaseCount != 1) || (blk.Header.height == 0 && coinbaseCount != 0) { return errors.Wrapf( ErrInvalidBlock, - "Wrong number of coinbase transfers") + "wrong number of coinbase transfers") } if correctAction+coinbaseCount != uint64(len(blk.Transfers)+len(blk.Votes)+len(blk.Executions)) { return errors.Wrapf( ErrInvalidBlock, - "Failed to verify actions signature") + "failed to verify actions signature") } if blk.Header.height > 0 { //Verify each account's Nonce