x/feegrant State machine audit

[blushi]

ref: #8983

• Read through MsgServer code and verify correctness upon visual inspection @blushi @aleem1314
• Ensure all state machine code which could be confusing is properly commented @blushi @aleem1314
• Make sure state machine logic matches Msg method documentation @blushi @aleem1314
• Ensure that all state machine edge cases are covered with tests and that test coverage is sufficient (at least 90% coverage on module code) @technicallyty @blushi
• Assess potential threats for each method including spam attacks and ensure that threats have been addressed sufficiently. This should be done by writing up threat assessment for each method. Specifically we should be paying attention to:
  • algorithmic complexity and places this could be exploited (ex. nested for loops)
  • charging gas complex computation (ex. for loops)
  • storage is safe (we don't pollute the state).
• Assess potential risks of any new third party dependencies and decide whether a dependency audit is needed @technicallyty @aleem1314
• Check correctness of simulation implementation if any (@aleem1314)

[aleem1314]

Simulations

genesis.go

• GenFeeGrants is used only in this package, so maybe it can be a private function.
• GenFeeGrants returning empty grants, it should generate some random grants.

operations.go

• types/TypeMsgGrantFeeAllowance, types/TypeMsgRevokeFeeAllowance are only used in simulations. May be let's move them to simulation/operations.go.
• Also in simulation/operations.go, Why we are using two msg-types? ( TypeMsgGrantFeeAllowance and types/TypeMsgGrantFeeAllowance )

operations_test.go

• Instead of hardcoding account address here, maybe we could use accounts.Address.

[blushi]

• Read through MsgServer code and verify correctness upon visual inspection

  • Why not getting the error from here?
  • Let's avoid panic inside keeper here. Lets use UnmarshalBinaryBare instead of MustUnmarshalBinaryBare .
  • Same here.
  • IterateAllGranteeFeeAllowances is defined, but not used anywhere.
  • IterateAllFeeAllowances is only used in simulations. Maybe we can use GetFeeGrant instead.

• Ensure all state machine code which could be confusing is properly commented

  • I don't find this comment very clear.

• Make sure state machine logic matches Msg method documentation

  • Not directly related to state machine code but called from it: (a FeeAllowanceGrant) GetFeeGrant() method naming is a bit confusing. Why not just use GetAllowance()?

• Ensure that all state machine edge cases are covered with tests and that test coverage is sufficient (at least 90% coverage on module code)

  • Keeper code (x/feegrant/keeper/keeper.go) covered by unit tests but not Msg server (x/feegrant/keeper/msg_server.go) get rid of most of the keeper public methods (except the one used in ante handler) and test msg_server methods instead
  • (k Keeper) RevokeFeeAllowance: add test case if fee grant not found
  • (k Keeper) GrantFeeAllowance: add test case with grantee account not yet in account state
  • Not directly related to the state machine tests, let's add unit tests to types/filtered_fee.go

[blushi]

• Assess potential threats for each method including spam attacks and ensure that threats have been addressed sufficiently. This should be done by writing up threat assessment for each method. Specifically we should be paying attention to: (pending on feegrant filtered msgs #8604)

  • algorithmic complexity and places this could be exploited (ex. nested for loops)
  • charging gas complex computation (ex. for loops)
    • We are not charging extra gas in IterateAllFeeAllowances. (Note: Currently this is only used in genesis export) We're already have store gas.
  • storage is safe (we don't pollute the state)
    • When a fee allowance grant expires, how do we get rid of it in the state? (maybe look at how proposals are handled in the gov module). Could be through EndBlocker or have a deposit that the creator of the grant would get back when deleting it, former solution preferred but this should be a separate task (it also makes sense for x/authz). Create a secondary index (expiration time + key).
    • I think x/authz pruning and gas #8311 applies here too.

• Assess potential risks of any new third party dependencies and decide whether a dependency audit is needed: no dependency other than gogo proto already used by other modules

[technicallyty]

• Ensure that all state machine edge cases are covered with tests and that test coverage is sufficient (at least 90% coverage on module code):

• In a previous issue we had removed support for time based upgrades. I think there may be a similar issue here, where we have FeeGrants involved around time. It would be nice to have a robust test around this

• In export genesis, we loop through the fee grants and diff the times relative to the 0 time. Is it possible for nodes to export the genesis with different timings? Does this break consensus?
  Get rid of block height in ExportGenesis and remove height support in general
  Get rid of PrepareForExport method from FeeAllowanceI iface

• In the file periodic_fee_test.go in test TestPeriodicFeeValidAllow a field blockTime exists in the test case struct, but is never used.

• Same as above for basic_fee_test.go in test TestBasicFeeValidAllow

• TestGrant only tests height, not time

• storage is safe (we don't pollute the state):

• Is there any mechanism to prune the state of old grants?

• Assess potential threats for each method including spam attacks and ensure that threats have been addressed sufficiently. This should be done by writing up threat assessment for each method. Specifically we should be paying attention to:
  • algorithmic complexity and places this could be exploited (ex. nested for loops)
  • no nested for loops
  • charging gas complex computation (ex. for loops)
  • the accept method charges gas accordingly