From 690b787fe30eb85af08f575b46bf10ffd9839f84 Mon Sep 17 00:00:00 2001 From: Evgeniy Stratonikov Date: Thu, 28 Jan 2021 18:01:30 +0300 Subject: [PATCH] native: unify committee checks Fail execution if tx is not signed by committee. --- pkg/core/native/management.go | 6 ++-- pkg/core/native/native_neo.go | 18 +++++------ pkg/core/native/notary.go | 6 ++-- pkg/core/native/policy.go | 40 ++++++++++++------------ pkg/core/native_neo_test.go | 56 ++-------------------------------- pkg/core/native_notary_test.go | 45 ++------------------------- pkg/core/native_policy_test.go | 16 +++++++--- 7 files changed, 51 insertions(+), 136 deletions(-) diff --git a/pkg/core/native/management.go b/pkg/core/native/management.go index 7cc9b3f21..76737d551 100644 --- a/pkg/core/native/management.go +++ b/pkg/core/native/management.go @@ -91,7 +91,7 @@ func newManagement() *Management { md = newMethodAndPrice(m.getMinimumDeploymentFee, 100_0000, callflag.ReadStates) m.AddMethod(md, desc) - desc = newDescriptor("setMinimumDeploymentFee", smartcontract.BoolType, + desc = newDescriptor("setMinimumDeploymentFee", smartcontract.VoidType, manifest.NewParameter("value", smartcontract.IntegerType)) md = newMethodAndPrice(m.setMinimumDeploymentFee, 300_0000, callflag.WriteStates) m.AddMethod(md, desc) @@ -369,13 +369,13 @@ func (m *Management) setMinimumDeploymentFee(ic *interop.Context, args []stackit panic(fmt.Errorf("MinimumDeploymentFee cannot be negative")) } if !m.NEO.checkCommittee(ic) { - return stackitem.NewBool(false) + panic("invalid committee signature") } err := setUint32WithKey(m.ContractID, ic.DAO, keyMinimumDeploymentFee, value) if err != nil { panic(err) } - return stackitem.NewBool(true) + return stackitem.Null{} } func (m *Management) callDeploy(ic *interop.Context, cs *state.Contract, isUpdate bool) { diff --git a/pkg/core/native/native_neo.go b/pkg/core/native/native_neo.go index 40967d0e2..d39100f83 100644 --- a/pkg/core/native/native_neo.go +++ b/pkg/core/native/native_neo.go @@ -145,7 +145,7 @@ func newNEO() *NEO { md = newMethodAndPrice(n.getGASPerBlock, 100_0000, callflag.ReadStates) n.AddMethod(md, desc) - desc = newDescriptor("setGasPerBlock", smartcontract.BoolType, + desc = newDescriptor("setGasPerBlock", smartcontract.VoidType, manifest.NewParameter("gasPerBlock", smartcontract.IntegerType)) md = newMethodAndPrice(n.setGASPerBlock, 500_0000, callflag.WriteStates) n.AddMethod(md, desc) @@ -472,25 +472,23 @@ func (n *NEO) checkCommittee(ic *interop.Context) bool { func (n *NEO) setGASPerBlock(ic *interop.Context, args []stackitem.Item) stackitem.Item { gas := toBigInt(args[0]) - ok, err := n.SetGASPerBlock(ic, ic.Block.Index+1, gas) + err := n.SetGASPerBlock(ic, ic.Block.Index+1, gas) if err != nil { panic(err) } - return stackitem.NewBool(ok) + return stackitem.Null{} } // SetGASPerBlock sets gas generated for blocks after index. -func (n *NEO) SetGASPerBlock(ic *interop.Context, index uint32, gas *big.Int) (bool, error) { +func (n *NEO) SetGASPerBlock(ic *interop.Context, index uint32, gas *big.Int) error { if gas.Sign() == -1 || gas.Cmp(big.NewInt(10*GASFactor)) == 1 { - return false, errors.New("invalid value for GASPerBlock") + return errors.New("invalid value for GASPerBlock") } - h := n.GetCommitteeAddress() - ok, err := runtime.CheckHashedWitness(ic, h) - if err != nil || !ok { - return ok, err + if !n.checkCommittee(ic) { + return errors.New("invalid committee signature") } n.gasPerBlockChanged.Store(true) - return true, n.putGASRecord(ic.DAO, index, gas) + return n.putGASRecord(ic.DAO, index, gas) } func (n *NEO) dropCandidateIfZero(d dao.DAO, pub *keys.PublicKey, c *candidate) (bool, error) { diff --git a/pkg/core/native/notary.go b/pkg/core/native/notary.go index 62510423d..0a27a5dec 100644 --- a/pkg/core/native/notary.go +++ b/pkg/core/native/notary.go @@ -93,7 +93,7 @@ func newNotary() *Notary { md = newMethodAndPrice(n.getMaxNotValidBeforeDelta, 100_0000, callflag.ReadStates) n.AddMethod(md, desc) - desc = newDescriptor("setMaxNotValidBeforeDelta", smartcontract.BoolType, + desc = newDescriptor("setMaxNotValidBeforeDelta", smartcontract.VoidType, manifest.NewParameter("value", smartcontract.IntegerType)) md = newMethodAndPrice(n.setMaxNotValidBeforeDelta, 300_0000, callflag.WriteStates) n.AddMethod(md, desc) @@ -389,7 +389,7 @@ func (n *Notary) setMaxNotValidBeforeDelta(ic *interop.Context, args []stackitem panic(fmt.Errorf("MaxNotValidBeforeDelta cannot be more than %d or less than %d", transaction.MaxValidUntilBlockIncrement/2, ic.Chain.GetConfig().ValidatorsCount)) } if !n.NEO.checkCommittee(ic) { - return stackitem.NewBool(false) + panic("invalid committee signature") } n.lock.Lock() defer n.lock.Unlock() @@ -398,7 +398,7 @@ func (n *Notary) setMaxNotValidBeforeDelta(ic *interop.Context, args []stackitem panic(fmt.Errorf("failed to put value into the storage: %w", err)) } n.isValid = false - return stackitem.NewBool(true) + return stackitem.Null{} } // GetDepositFor returns state.Deposit for the account specified. It returns nil in case if diff --git a/pkg/core/native/policy.go b/pkg/core/native/policy.go index 98fc26c1d..4afdef5ce 100644 --- a/pkg/core/native/policy.go +++ b/pkg/core/native/policy.go @@ -109,7 +109,7 @@ func newPolicy() *Policy { md = newMethodAndPrice(p.getExecFeeFactor, 1000000, callflag.ReadStates) p.AddMethod(md, desc) - desc = newDescriptor("setExecFeeFactor", smartcontract.BoolType, + desc = newDescriptor("setExecFeeFactor", smartcontract.VoidType, manifest.NewParameter("value", smartcontract.IntegerType)) md = newMethodAndPrice(p.setExecFeeFactor, 3000000, callflag.WriteStates) p.AddMethod(md, desc) @@ -118,27 +118,27 @@ func newPolicy() *Policy { md = newMethodAndPrice(p.getStoragePrice, 1000000, callflag.ReadStates) p.AddMethod(md, desc) - desc = newDescriptor("setStoragePrice", smartcontract.BoolType, + desc = newDescriptor("setStoragePrice", smartcontract.VoidType, manifest.NewParameter("value", smartcontract.IntegerType)) md = newMethodAndPrice(p.setStoragePrice, 3000000, callflag.WriteStates) p.AddMethod(md, desc) - desc = newDescriptor("setMaxBlockSize", smartcontract.BoolType, + desc = newDescriptor("setMaxBlockSize", smartcontract.VoidType, manifest.NewParameter("value", smartcontract.IntegerType)) md = newMethodAndPrice(p.setMaxBlockSize, 3000000, callflag.WriteStates) p.AddMethod(md, desc) - desc = newDescriptor("setMaxTransactionsPerBlock", smartcontract.BoolType, + desc = newDescriptor("setMaxTransactionsPerBlock", smartcontract.VoidType, manifest.NewParameter("value", smartcontract.IntegerType)) md = newMethodAndPrice(p.setMaxTransactionsPerBlock, 3000000, callflag.WriteStates) p.AddMethod(md, desc) - desc = newDescriptor("setFeePerByte", smartcontract.BoolType, + desc = newDescriptor("setFeePerByte", smartcontract.VoidType, manifest.NewParameter("value", smartcontract.IntegerType)) md = newMethodAndPrice(p.setFeePerByte, 3000000, callflag.WriteStates) p.AddMethod(md, desc) - desc = newDescriptor("setMaxBlockSystemFee", smartcontract.BoolType, + desc = newDescriptor("setMaxBlockSystemFee", smartcontract.VoidType, manifest.NewParameter("value", smartcontract.IntegerType)) md = newMethodAndPrice(p.setMaxBlockSystemFee, 3000000, callflag.WriteStates) p.AddMethod(md, desc) @@ -309,7 +309,7 @@ func (p *Policy) setExecFeeFactor(ic *interop.Context, args []stackitem.Item) st panic(fmt.Errorf("ExecFeeFactor must be between 0 and %d", maxExecFeeFactor)) } if !p.NEO.checkCommittee(ic) { - return stackitem.NewBool(false) + panic("invalid committee signature") } p.lock.Lock() defer p.lock.Unlock() @@ -318,7 +318,7 @@ func (p *Policy) setExecFeeFactor(ic *interop.Context, args []stackitem.Item) st panic(err) } p.isValid = false - return stackitem.NewBool(true) + return stackitem.Null{} } // isBlocked is Policy contract method and checks whether provided account is blocked. @@ -365,7 +365,7 @@ func (p *Policy) setStoragePrice(ic *interop.Context, args []stackitem.Item) sta panic(fmt.Errorf("StoragePrice must be between 0 and %d", maxStoragePrice)) } if !p.NEO.checkCommittee(ic) { - return stackitem.NewBool(false) + panic("invalid committee signature") } p.lock.Lock() defer p.lock.Unlock() @@ -374,7 +374,7 @@ func (p *Policy) setStoragePrice(ic *interop.Context, args []stackitem.Item) sta panic(err) } p.isValid = false - return stackitem.NewBool(true) + return stackitem.Null{} } // setMaxTransactionsPerBlock is Policy contract method and sets the upper limit @@ -385,7 +385,7 @@ func (p *Policy) setMaxTransactionsPerBlock(ic *interop.Context, args []stackite panic(fmt.Errorf("MaxTransactionsPerBlock cannot exceed the maximum allowed transactions per block = %d", block.MaxTransactionsPerBlock)) } if !p.NEO.checkCommittee(ic) { - return stackitem.NewBool(false) + panic("invalid committee signature") } p.lock.Lock() defer p.lock.Unlock() @@ -394,7 +394,7 @@ func (p *Policy) setMaxTransactionsPerBlock(ic *interop.Context, args []stackite panic(err) } p.isValid = false - return stackitem.NewBool(true) + return stackitem.Null{} } // setMaxBlockSize is Policy contract method and sets maximum block size. @@ -404,7 +404,7 @@ func (p *Policy) setMaxBlockSize(ic *interop.Context, args []stackitem.Item) sta panic(fmt.Errorf("MaxBlockSize cannot be more than the maximum payload size = %d", payload.MaxSize)) } if !p.NEO.checkCommittee(ic) { - return stackitem.NewBool(false) + panic("invalid committee signature") } p.lock.Lock() defer p.lock.Unlock() @@ -413,7 +413,7 @@ func (p *Policy) setMaxBlockSize(ic *interop.Context, args []stackitem.Item) sta panic(err) } p.isValid = false - return stackitem.NewBool(true) + return stackitem.Null{} } // setFeePerByte is Policy contract method and sets transaction's fee per byte. @@ -423,7 +423,7 @@ func (p *Policy) setFeePerByte(ic *interop.Context, args []stackitem.Item) stack panic(fmt.Errorf("FeePerByte shouldn't be negative or greater than %d", maxFeePerByte)) } if !p.NEO.checkCommittee(ic) { - return stackitem.NewBool(false) + panic("invalid committee signature") } p.lock.Lock() defer p.lock.Unlock() @@ -432,7 +432,7 @@ func (p *Policy) setFeePerByte(ic *interop.Context, args []stackitem.Item) stack panic(err) } p.isValid = false - return stackitem.NewBool(true) + return stackitem.Null{} } // setMaxBlockSystemFee is Policy contract method and sets the maximum system fee per block. @@ -442,7 +442,7 @@ func (p *Policy) setMaxBlockSystemFee(ic *interop.Context, args []stackitem.Item panic(fmt.Errorf("MaxBlockSystemFee cannot be less then %d", minBlockSystemFee)) } if !p.NEO.checkCommittee(ic) { - return stackitem.NewBool(false) + panic("invalid committee signature") } p.lock.Lock() defer p.lock.Unlock() @@ -451,14 +451,14 @@ func (p *Policy) setMaxBlockSystemFee(ic *interop.Context, args []stackitem.Item panic(err) } p.isValid = false - return stackitem.NewBool(true) + return stackitem.Null{} } // blockAccount is Policy contract method and adds given account hash to the list // of blocked accounts. func (p *Policy) blockAccount(ic *interop.Context, args []stackitem.Item) stackitem.Item { if !p.NEO.checkCommittee(ic) { - return stackitem.NewBool(false) + panic("invalid committee signature") } hash := toUint160(args[0]) if p.IsBlockedInternal(ic.DAO, hash) { @@ -481,7 +481,7 @@ func (p *Policy) blockAccount(ic *interop.Context, args []stackitem.Item) stacki // the list of blocked accounts. func (p *Policy) unblockAccount(ic *interop.Context, args []stackitem.Item) stackitem.Item { if !p.NEO.checkCommittee(ic) { - return stackitem.NewBool(false) + panic("invalid committee signature") } hash := toUint160(args[0]) if !p.IsBlockedInternal(ic.DAO, hash) { diff --git a/pkg/core/native_neo_test.go b/pkg/core/native_neo_test.go index 9d7d87ea5..ea3d45410 100644 --- a/pkg/core/native_neo_test.go +++ b/pkg/core/native_neo_test.go @@ -197,57 +197,8 @@ func TestNEO_SetGasPerBlock(t *testing.T) { bc := newTestChain(t) defer bc.Close() - neo := bc.contracts.NEO - tx := transaction.New(netmode.UnitTestNet, []byte{}, 0) - ic := bc.newInteropContext(trigger.Application, bc.dao, nil, tx) - ic.SpawnVM() - ic.VM.LoadScript([]byte{byte(opcode.RET)}) - - h := neo.GetCommitteeAddress() - t.Run("Default", func(t *testing.T) { - g := neo.GetGASPerBlock(ic.DAO, 0) - require.EqualValues(t, 5*native.GASFactor, g.Int64()) - }) - t.Run("Invalid", func(t *testing.T) { - t.Run("InvalidSignature", func(t *testing.T) { - setSigner(tx, util.Uint160{}) - ok, err := neo.SetGASPerBlock(ic, 10, big.NewInt(native.GASFactor)) - require.NoError(t, err) - require.False(t, ok) - }) - t.Run("TooBigValue", func(t *testing.T) { - setSigner(tx, h) - _, err := neo.SetGASPerBlock(ic, 10, big.NewInt(10*native.GASFactor+1)) - require.Error(t, err) - }) - }) - t.Run("Valid", func(t *testing.T) { - setSigner(tx, h) - ok, err := neo.SetGASPerBlock(ic, 10, big.NewInt(native.GASFactor*2)) - require.NoError(t, err) - require.True(t, ok) - _, err = ic.DAO.Persist() - require.NoError(t, err) - - t.Run("Again", func(t *testing.T) { - setSigner(tx, h) - ok, err := neo.SetGASPerBlock(ic, 10, big.NewInt(native.GASFactor)) - require.NoError(t, err) - require.True(t, ok) - - t.Run("NotPersisted", func(t *testing.T) { - g := neo.GetGASPerBlock(bc.dao, 10) - // Old value should be returned. - require.EqualValues(t, 2*native.GASFactor, g.Int64()) - }) - }) - - g := neo.GetGASPerBlock(ic.DAO, 9) - require.EqualValues(t, 5*native.GASFactor, g.Int64()) - - g = neo.GetGASPerBlock(ic.DAO, 10) - require.EqualValues(t, native.GASFactor, g.Int64()) - }) + testGetSet(t, bc, bc.contracts.NEO.Hash, "GasPerBlock", + 5*native.GASFactor, 0, 10*native.GASFactor) } func TestNEO_CalculateBonus(t *testing.T) { @@ -270,9 +221,8 @@ func TestNEO_CalculateBonus(t *testing.T) { }) t.Run("ManyBlocks", func(t *testing.T) { setSigner(tx, neo.GetCommitteeAddress()) - ok, err := neo.SetGASPerBlock(ic, 10, big.NewInt(1*native.GASFactor)) + err := neo.SetGASPerBlock(ic, 10, big.NewInt(1*native.GASFactor)) require.NoError(t, err) - require.True(t, ok) res, err := neo.CalculateNEOHolderReward(ic.DAO, big.NewInt(100), 5, 15) require.NoError(t, err) diff --git a/pkg/core/native_notary_test.go b/pkg/core/native_notary_test.go index bb287851c..d72c305ce 100644 --- a/pkg/core/native_notary_test.go +++ b/pkg/core/native_notary_test.go @@ -2,14 +2,12 @@ package core import ( "math" - "math/big" "testing" "github.com/nspcc-dev/neo-go/internal/testchain" "github.com/nspcc-dev/neo-go/pkg/core/native" "github.com/nspcc-dev/neo-go/pkg/core/transaction" "github.com/nspcc-dev/neo-go/pkg/crypto/keys" - "github.com/nspcc-dev/neo-go/pkg/encoding/bigint" "github.com/nspcc-dev/neo-go/pkg/io" "github.com/nspcc-dev/neo-go/pkg/smartcontract/callflag" "github.com/nspcc-dev/neo-go/pkg/smartcontract/trigger" @@ -329,46 +327,7 @@ func TestNotaryNodesReward(t *testing.T) { func TestMaxNotValidBeforeDelta(t *testing.T) { chain := newTestChain(t) defer chain.Close() - notaryHash := chain.contracts.Notary.Hash - transferFundsToCommittee(t, chain) - - t.Run("get, internal method", func(t *testing.T) { - n := chain.contracts.Notary.GetMaxNotValidBeforeDelta(chain.dao) - require.Equal(t, 140, int(n)) - }) - - t.Run("get, contract method", func(t *testing.T) { - res, err := invokeContractMethod(chain, 100000000, notaryHash, "getMaxNotValidBeforeDelta") - require.NoError(t, err) - checkResult(t, res, stackitem.NewBigInteger(big.NewInt(140))) - require.NoError(t, chain.persist()) - }) - - t.Run("set", func(t *testing.T) { - res, err := invokeContractMethodGeneric(chain, 100000000, notaryHash, "setMaxNotValidBeforeDelta", true, bigint.ToBytes(big.NewInt(150))) - require.NoError(t, err) - checkResult(t, res, stackitem.NewBool(true)) - n := chain.contracts.Notary.GetMaxNotValidBeforeDelta(chain.dao) - require.Equal(t, 150, int(n)) - }) - - t.Run("set, too big value", func(t *testing.T) { - res, err := invokeContractMethodGeneric(chain, 100000000, notaryHash, "setMaxNotValidBeforeDelta", true, bigint.ToBytes(big.NewInt(transaction.MaxValidUntilBlockIncrement/2+1))) - require.NoError(t, err) - checkFAULTState(t, res) - }) - - t.Run("set, too small value", func(t *testing.T) { - res, err := invokeContractMethodGeneric(chain, 100000000, notaryHash, "setMaxNotValidBeforeDelta", true, bigint.ToBytes(big.NewInt(int64(chain.GetConfig().ValidatorsCount-1)))) - require.NoError(t, err) - checkFAULTState(t, res) - }) - - t.Run("set, not signed by committee", func(t *testing.T) { - signer, err := wallet.NewAccount() - require.NoError(t, err) - invokeRes, err := invokeContractMethodBy(t, chain, signer, notaryHash, "setMaxNotValidBeforeDelta", bigint.ToBytes(big.NewInt(150))) - checkResult(t, invokeRes, stackitem.NewBool(false)) - }) + testGetSet(t, chain, chain.contracts.Notary.Hash, "MaxNotValidBeforeDelta", + 140, int64(chain.GetConfig().ValidatorsCount), transaction.MaxValidUntilBlockIncrement/2) } diff --git a/pkg/core/native_policy_test.go b/pkg/core/native_policy_test.go index 244eccc5c..b8020d037 100644 --- a/pkg/core/native_policy_test.go +++ b/pkg/core/native_policy_test.go @@ -29,7 +29,8 @@ func testGetSet(t *testing.T, chain *Blockchain, hash util.Uint160, name string, signer, err := wallet.NewAccount() require.NoError(t, err) invokeRes, err := invokeContractMethodBy(t, chain, signer, hash, setName, minValue+1) - checkResult(t, invokeRes, stackitem.NewBool(false)) + require.NoError(t, err) + checkFAULTState(t, invokeRes) }) t.Run("get, defult value", func(t *testing.T) { @@ -61,8 +62,10 @@ func testGetSet(t *testing.T, chain *Blockchain, hash util.Uint160, name string, require.NoError(t, err) aers, err := persistBlock(chain, txSet, txGet1) require.NoError(t, err) - checkResult(t, aers[0], stackitem.NewBool(true)) - checkResult(t, aers[1], stackitem.Make(defaultValue+1)) + checkResult(t, aers[0], stackitem.Null{}) + if name != "GasPerBlock" { // GasPerBlock is set on the next block + checkResult(t, aers[1], stackitem.Make(defaultValue+1)) + } require.NoError(t, chain.persist()) // Get in the next block. @@ -214,6 +217,11 @@ func TestBlockedAccounts(t *testing.T) { signer, err := wallet.NewAccount() require.NoError(t, err) invokeRes, err := invokeContractMethodBy(t, chain, signer, policyHash, "blockAccount", account.BytesBE()) - checkResult(t, invokeRes, stackitem.NewBool(false)) + require.NoError(t, err) + checkFAULTState(t, invokeRes) + + invokeRes, err = invokeContractMethodBy(t, chain, signer, policyHash, "unblockAccount", account.BytesBE()) + require.NoError(t, err) + checkFAULTState(t, invokeRes) }) }