ENG 450 feesplit state machine audit #715

danburck · 2022-06-23T16:01:08Z

Description

This PR adds improvements from performing a state machine audit on the x/feesplit module. Here are some changes to point out:

Module rename

x/fees -> x/feesplit

KV store Changes

fee_split_prefix | contract_address -> fees
deployer_prefix | deployer_address | contract_address -> byte{1}
withdraw_prefix | withdraw_address | contract_address -> byte{1}

UpdateFee msg

Sets withdraw address to "" if the withdraw address is empty or the same as deployer address

Closes ENG-450: https://linear.app/evmos/issue/ENG-450/state-machine-audit

x/fees/keeper/msg_server.go

x/fees/keeper/grpc_query.go

x/fees/keeper/evm_hooks.go

x/fees/keeper/fees.go

…/evmos/evmos into ENG-450-fees-state-machine-audit

x/fees/genesis.go

x/fees/keeper/evm_hooks.go

x/fees/keeper/msg_server_test.go

codecov · 2022-06-25T13:25:46Z

Codecov Report

Merging #715 (f356dfe) into main (877683c) will decrease coverage by 0.21%.
The diff coverage is 90.13%.

❗ Current head f356dfe differs from pull request most recent head be9932d. Consider uploading reports for the commit be9932d to get more accurate results

@@            Coverage Diff             @@
##             main     #715      +/-   ##
==========================================
- Coverage   81.50%   81.29%   -0.22%     
==========================================
  Files         123      123              
  Lines        7013     7120     +107     
==========================================
+ Hits         5716     5788      +72     
- Misses       1159     1190      +31     
- Partials      138      142       +4

Impacted Files	Coverage Δ
x/feesplit/keeper/keeper.go	`100.00% <ø> (ø)`
x/feesplit/keeper/params.go	`100.00% <ø> (ø)`
x/feesplit/types/keys.go	`0.00% <0.00%> (ø)`
x/feesplit/types/fee_split.go	`70.96% <70.96%> (ø)`
x/feesplit/types/params.go	`94.64% <75.00%> (ø)`
x/erc20/keeper/evm_hooks.go	`88.88% <84.61%> (+0.28%)`	⬆️
x/feesplit/keeper/msg_server.go	`86.84% <86.84%> (ø)`
x/feesplit/keeper/grpc_query.go	`88.18% <88.18%> (ø)`
x/feesplit/keeper/fee_splits.go	`97.14% <97.14%> (ø)`
app/app.go	`85.21% <100.00%> (ø)`
... and 6 more

MalteHerrmann

Changes look good to me! Minor comments on the docs table and what I believe to be a dev line.

Thank you for incorporating several of my suggestions that fast!

app/ante/ante.go

x/fees/spec/02_state.md

…/evmos/evmos into ENG-450-fees-state-machine-audit

proto/evmos/feesplit/v1/genesis.proto

data/application.db/CURRENT

third_party/proto/cosmos/distribution/v1beta1/query.proto

third_party/proto/cosmos/distribution/v1beta1/tx.proto

x/feesplit/spec/05_hooks.md

fedekunze

LGTM, minor suggestions

third_party/proto/cosmos/distribution/v1beta1/query.proto

x/feesplit/client/cli/query.go

x/feesplit/client/cli/tx.go

x/feesplit/keeper/evm_hooks.go

x/feesplit/keeper/msg_server.go

x/feesplit/keeper/msg_server_test.go

x/feesplit/spec/08_clients.md

mojtaba-esk · 2022-07-19T07:07:39Z

x/claims/keeper/hooks.go

-// AfterEVMStateTransition implements the ethermint evm PostTxProcessing hook.
+// PostTxProcessing is a wrapper for calling the EVM PostTxProcessing hook on
+// the module keeper
+func (h Hooks) PostTxProcessing(ctx sdk.Context, msg core.Message, receipt *ethtypes.Receipt) error {


Nit: would not it be more consistent if we use the prefix After instead of Post? just like what used for other hooks.

mojtaba-esk · 2022-07-19T09:28:38Z

x/feesplit/types/msg.go

-			return sdkerrors.Wrapf(err, "invalid withdraw address address %s", msg.WithdrawAddress)
+	if msg.WithdrawerAddress != "" {
+		if _, err := sdk.AccAddressFromBech32(msg.WithdrawerAddress); err != nil {
+			return sdkerrors.Wrapf(err, "invalid withdraw address %s", msg.WithdrawerAddress)


Nit: A little typo in invalid withdraw address %s according to the new changes, it should be withdrawer, right?

GAtom22

LGTM! There are some cases where I believe the naming consistency is compromised. Please, check the comments left about this. Also, it would have been nicer to have this refactor broken down into 2 or 3 different PRs to make it easier for review.

GAtom22 · 2022-09-08T22:06:12Z

x/feesplit/spec/01_concepts.md


 Users pay transaction fees to pay interact with smart contracts using the EVM. When a transaction is executed, the entire fee amount (`gasLimit * gasPrice`) is sent to the `FeeCollector` module account during the [Cosmos SDK AnteHandler](https://docs.cosmos.network/v0.44/modules/auth/03_antehandlers.html) execution. After the EVM executes the transaction, the user receives a refund of `(gasLimit - gasUsed) * gasPrice`. In result a user pays a total transaction fee of `txFee = gasUsed * gasPrice` for the execution.


I believe there's an extra word in the first line. In the phrase "Users pay transaction fees to pay interact with smart contracts ...". Should we remove the pay there?

GAtom22 · 2022-09-08T22:37:34Z

x/feesplit/spec/04_transactions.md

-	WithdrawAddress string `protobuf:"bytes,3,opt,name=withdraw_address,json=withdrawAddress,proto3" json:"withdraw_address,omitempty"`
+	WithdrawerAddress string `protobuf:"bytes,3,opt,name=withdraw_address,json=withdrawerAddress,proto3" json:"withdraw_address,omitempty"`


According to the code in tx.pb.go, the tag fields name and json should also be updated to withdrawer_address. This line should be:

WithdrawerAddress string `protobuf:"bytes,3,opt,name=withdrawer_address,json=withdrawerAddress,proto3" json:"withdrawer_address,omitempty"`

GAtom22 · 2022-09-08T22:44:01Z

x/feesplit/types/errors.go

+
+// errors
+var (
+	ErrInternalFeeSplit              = sdkerrors.Register(ModuleName, 2, "internal feesplit error")


Nit: to keep the same naming convention as the other errors, we could change this to ErrFeeSplitInternal

GAtom22 · 2022-09-08T22:49:15Z

x/feesplit/types/fee_split.go

@@ -0,0 +1,62 @@
+package types


Nit: to keep the naming convention with the package name that contains this package, we could refactor this file name to feesplit.go

GAtom22 · 2022-09-08T22:56:47Z

x/feesplit/types/fee_split.go

+// GetContractAddr returns the contract address
+func (fs FeeSplit) GetContractAddr() common.Address {
+	return common.HexToAddress(fs.ContractAddress)
+}
+
+// GetDeployerAddr returns the contract deployer address
+func (fs FeeSplit) GetDeployerAddr() sdk.AccAddress {
+	return sdk.MustAccAddressFromBech32(fs.DeployerAddress)
+}
+
+// GetWithdrawerAddr returns the account address to where the funds proceeding
+// from the fees will be received. If the withdraw address is not defined, it
+// defaults to the deployer address.
+func (fs FeeSplit) GetWithdrawerAddr() sdk.AccAddress {


Nit: according to some opinions about best practices in Golang, getter methods should be called by the field name with upper case. For example, the method name should be ContractAddr instead of GetContractAddr

GAtom22 · 2022-09-09T00:10:58Z

x/feesplit/keeper/grpc_query_test.go

+				req = &types.QueryFeeSplitsRequest{
+					Pagination: &query.PageRequest{Limit: 10, CountTotal: true},
+				}
+				feeSplit := types.NewFeeSplit(contract, deployer, withdraw)


Nil: refactor withdraw param to withdrawer

GAtom22 · 2022-09-09T00:11:47Z

x/feesplit/keeper/grpc_query_test.go

+		{
+			"fee info found",
+			func() {
+				feeSplit := types.NewFeeSplit(contract, deployer, withdraw)


Nil: Idem as above, refactor to withdrawer

GAtom22 · 2022-09-09T00:13:49Z

x/feesplit/keeper/integration_test.go

 				contractAddress,
 				deployerAddress,
-				deployerAddress,
+				withdraw,


Nil: refactor var name to withdrawer

GAtom22 · 2022-09-09T00:59:29Z

x/feesplit/types/tx.pb.go

 }

-func (m *MsgRegisterFee) Marshal() (dAtA []byte, err error) {
+func (m *MsgRegisterFeeSplit) Marshal() (dAtA []byte, err error) {


Refactor parameter name dAtA to data for better naming consistency

GAtom22 · 2022-09-09T01:00:48Z

x/feesplit/types/tx.pb.go

@@ -860,7 +861,7 @@ func sovTx(x uint64) (n int) {
 func sozTx(x uint64) (n int) {
 	return sovTx(uint64((x << 1) ^ uint64((int64(x) >> 63))))
 }
-func (m *MsgRegisterFee) Unmarshal(dAtA []byte) error {
+func (m *MsgRegisterFeeSplit) Unmarshal(dAtA []byte) error {
 	l := len(dAtA)
 	iNdEx := 0


Refactor var name iNdEx to index for better naming consistency

K1-R1

LGTM. This CL looks good to me; other than the instances of naming inconsistency already pointed out above, I only additionally identified some minor grammar corrections in the docs.

K1-R1 · 2022-09-13T16:12:22Z

x/feesplit/spec/01_concepts.md

@@ -23,7 +23,7 @@ As described above, developers will earn a portion of the transaction fee after



Nit: docs grammar for Fee Distribution (not in CL);
The registration of factory contracts (smart contracts that have been deployed by other contracts) requires the identification of the original contract's deployer. This is done through address derivation.

K1-R1 · 2022-09-13T16:16:58Z

x/feesplit/spec/01_concepts.md

@@ -23,7 +23,7 @@ As described above, developers will earn a portion of the transaction fee after

 Users pay transaction fees to pay interact with smart contracts using the EVM. When a transaction is executed, the entire fee amount (`gasLimit * gasPrice`) is sent to the `FeeCollector` module account during the [Cosmos SDK AnteHandler](https://docs.cosmos.network/v0.44/modules/auth/03_antehandlers.html) execution. After the EVM executes the transaction, the user receives a refund of `(gasLimit - gasUsed) * gasPrice`. In result a user pays a total transaction fee of `txFee = gasUsed * gasPrice` for the execution.

-This transaction fee is distributed between developers and validators, in accordance with the `x/fees` module parameters: `DeveloperShares`, `ValidatorShares`. This distribution is handled through the EVM's [`PostTxProcessing` Hook](./05_hooks.md).
+This transaction fee is distributed between developers and validators, in accordance with the `x/feesplit` module parameters: `DeveloperShares`, `ValidatorShares`. This distribution is handled through the EVM's [`PostTxProcessing` Hook](./05_hooks.md).

 ### Address Derivation



Nit: docs grammar for Address Derivation (not in CL);
In both cases, the fee distribution requires the identification of a deployer address that is an EOA address, unless a withdrawal address is set by the contract deployer during registration to receive transaction fees for a registered smart contract.

K1-R1 · 2022-09-13T16:19:33Z

x/feesplit/spec/04_transactions.md

@@ -6,19 +6,19 @@ order: 4

 This section defines the `sdk.Msg` concrete types that result in the state transitions defined on the previous section.


Nit: docs grammar for intro;
This section defines the sdk.Msg concrete types that result in the state transitions defined in the previous section.

danburck and others added 11 commits June 17, 2022 10:17

wip msg_server.go

8b495e0

audit(fees): improve err messages in msg_server

c68fe33

Merge branch 'main' into ENG-450-fees-state-machine-audit

587099f

audit(fees): state machine audit (#716)

a2f8424

audit(fees): wip prefix refactoring

7902f86

audit(fees): wip prefix refactoring msg_server and hooks

149ef37

audit(fees): wip fixing tests

88eca15

audit(fees): fix tests

234c65c

audit(fees): remove old prefix getters,setters

dbb3ac8

go mod tidy

236fe70

audit(fees): fix specs

7249f21

fedekunze reviewed Jun 24, 2022

View reviewed changes

danburck added 4 commits June 24, 2022 18:38

audit(fees): add withdrawFees grpc tests

d13db8e

address comments

8926bd6

Merge branch 'main' into ENG-450-fees-state-machine-audit

406676f

Merge branch 'ENG-450-fees-state-machine-audit' of https://github.com…

d20d8a6

…/evmos/evmos into ENG-450-fees-state-machine-audit

danburck commented Jun 24, 2022

View reviewed changes

x/fees/genesis.go Outdated Show resolved Hide resolved

danburck added 2 commits June 24, 2022 19:45

update spec and changelog

aed42e3

small fixes

d3d2d34

danburck marked this pull request as ready for review June 24, 2022 17:56

danburck requested review from ramacarlucho, hanchon, khoslaventures and jolube as code owners June 24, 2022 17:56

fedekunze reviewed Jun 24, 2022

View reviewed changes

x/fees/genesis.go Outdated Show resolved Hide resolved

x/fees/keeper/evm_hooks.go Outdated Show resolved Hide resolved

x/fees/keeper/msg_server_test.go Show resolved Hide resolved

Merge branch 'main' into ENG-450-fees-state-machine-audit

b0e45b3

github-actions bot added CLI proto docs labels Jun 25, 2022

danburck added 2 commits July 4, 2022 17:42

wip address comments

ca41838

address Malte`s comments

f108121

MalteHerrmann reviewed Jul 4, 2022

View reviewed changes

app/ante/ante.go Outdated Show resolved Hide resolved

x/fees/spec/02_state.md Outdated Show resolved Hide resolved

danburck added 5 commits July 5, 2022 16:46

feesplitt(naming): rename fees module to feesplit

c979790

Merge branch 'main' into ENG-450-fees-state-machine-audit

3d171dc

feesplit(naming): update changelog

f395946

Merge branch 'ENG-450-fees-state-machine-audit' of https://github.com…

cc4329e

…/evmos/evmos into ENG-450-fees-state-machine-audit

feesplit(naming): update specs

1f56ebe

danburck changed the title ~~ENG 450 fees state machine audit~~ ENG 450 feesplit state machine audit Jul 5, 2022

ramacarlucho reviewed Jul 5, 2022

View reviewed changes

feesplit(naming): remove unwanted files

b6f1a5a

fedekunze approved these changes Jul 6, 2022

View reviewed changes

danburck added 11 commits July 6, 2022 14:13

feesplit(naming): remove unwanted withdrawer changes

0c09446

feesplit(naming): make proto-gen

0957cfa

feesplit(naming): remove more unwanted changes

137592f

feesplit(cli): improve command names

e74276b

feesplit(hooks): improve hook wrapper comments

e83c1f1

feesplit(hooks): fix tests

533e901

feesplit(rename): address last comments

4036003

feesplit(keeper): improve fees fucntion comments

0e0ff97

feesplit(keeper): rename fee variables to feeSplit

db7671e

feesplit(comments): fix comments

7279bc6

Merge branch 'main' into ENG-450-fees-state-machine-audit

f356dfe

danburck enabled auto-merge (squash) July 6, 2022 14:20

fix golangli-lint

be9932d

danburck merged commit 0d6c04b into main Jul 6, 2022

danburck deleted the ENG-450-fees-state-machine-audit branch July 6, 2022 14:37

mojtaba-esk reviewed Jul 19, 2022

View reviewed changes

GAtom22 reviewed Sep 9, 2022

View reviewed changes

K1-R1 reviewed Sep 13, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENG 450 feesplit state machine audit #715

ENG 450 feesplit state machine audit #715

danburck commented Jun 23, 2022 •

edited

Loading

codecov bot commented Jun 25, 2022 •

edited

Loading

MalteHerrmann left a comment

fedekunze left a comment

mojtaba-esk Jul 19, 2022

mojtaba-esk Jul 19, 2022

GAtom22 left a comment

GAtom22 Sep 8, 2022

GAtom22 Sep 8, 2022

GAtom22 Sep 8, 2022

GAtom22 Sep 8, 2022

GAtom22 Sep 8, 2022

GAtom22 Sep 9, 2022

GAtom22 Sep 9, 2022

GAtom22 Sep 9, 2022

GAtom22 Sep 9, 2022

GAtom22 Sep 9, 2022

K1-R1 left a comment •

edited

Loading

K1-R1 Sep 13, 2022

K1-R1 Sep 13, 2022

K1-R1 Sep 13, 2022


		Users pay transaction fees to pay interact with smart contracts using the EVM. When a transaction is executed, the entire fee amount (`gasLimit * gasPrice`) is sent to the `FeeCollector` module account during the [Cosmos SDK AnteHandler](https://docs.cosmos.network/v0.44/modules/auth/03_antehandlers.html) execution. After the EVM executes the transaction, the user receives a refund of `(gasLimit - gasUsed) * gasPrice`. In result a user pays a total transaction fee of `txFee = gasUsed * gasPrice` for the execution.

		WithdrawAddress string `protobuf:"bytes,3,opt,name=withdraw_address,json=withdrawAddress,proto3" json:"withdraw_address,omitempty"`
		WithdrawerAddress string `protobuf:"bytes,3,opt,name=withdraw_address,json=withdrawerAddress,proto3" json:"withdraw_address,omitempty"`

		@@ -23,7 +23,7 @@ As described above, developers will earn a portion of the transaction fee after

		@@ -6,19 +6,19 @@ order: 4

		This section defines the `sdk.Msg` concrete types that result in the state transitions defined on the previous section.

ENG 450 feesplit state machine audit #715

ENG 450 feesplit state machine audit #715

Conversation

danburck commented Jun 23, 2022 • edited Loading

Description

Module rename

KV store Changes

UpdateFee msg

codecov bot commented Jun 25, 2022 • edited Loading

Codecov Report

MalteHerrmann left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GAtom22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K1-R1 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danburck commented Jun 23, 2022 •

edited

Loading

codecov bot commented Jun 25, 2022 •

edited

Loading

K1-R1 left a comment •

edited

Loading