fix: audit remediations (#569)

* ensure cwerrors endblocker runs after callback and cwica

* remove unused and fix typos

* cwica related documentation updates

* cwerrors   related documentation updates

* Update CHANGELOG.md

CHANGELOG.md

@@ -39,6 +39,7 @@ Contains all the PRs that improved the code without changing the behaviors.
 ### Removed
 
 ### Fixed
+- [#569](https://github.com/archway-network/archway/pull/569) - Audit remidiations for x/cwerrors and x/cwica 
 
 ### Improvements
 - [#567](https://github.com/archway-network/archway/pull/567) - Remove redundant params fetching in SaveCallback 

app/app.go

@@ -786,11 +786,12 @@ func NewArchwayApp(
 		trackingTypes.ModuleName,
 		rewardsTypes.ModuleName,
 		callbackTypes.ModuleName,
-		cwerrorsTypes.ModuleName,
 		// invariants checks are always the last to run
 		crisistypes.ModuleName,
 		cwfees.ModuleName, // does not have end blocker
 		cwicatypes.ModuleName,
+
+		cwerrorsTypes.ModuleName, // should be after all the other cw modules
 	)
 
 	// NOTE: The genutils module must occur after staking so that pools are

x/cwerrors/spec/03_end_block.md

@@ -14,4 +14,4 @@ All the contract subscriptions which end in the current block are pruned from st
 
 ## Prune old errors
 
-All errors which are marked for deletion for the current block height are pruned from the state.
+All errors which haven been associated with the current block height as the expiration height are pruned from the state

x/cwerrors/spec/README.md

@@ -10,7 +10,7 @@ The module provides two diffferent ways that the error is communicated.
 
 ### 1. Errors saved in state (default)
 
-Whenever a contract relevant error is encountered by the protocol, the module stores the error and associates with the contract address. This is stored in the chain state for `x` number of blocks (The `x` value is a module parameter. [See more](./01_state.md)) after which the error is permanently deleted. These stored errors are queryable by the contract using stargate queries.
+Whenever a contract relevant error is encountered by the protocol, the module stores the error and associates with the contract address. This is stored in the chain state for `x` number of blocks (The `x` value is a module parameter. [See more](./01_state.md)) after which the error is automatically pruned. These stored errors are queryable by the contract using stargate queries.
 
 ### 2. Errors sudo callback
 
@@ -31,6 +31,33 @@ pub enum SudoMsg  {
 
 The subscriptions are an opt-in feature where the contractadmin/owner has to subscribe to the feature by paying the relevant fees. [See more](01_state.md) The subscription is valid for `x` number of blocks where `x` is decided by the module param. The subscription cannot be cancelled but can be extended by attempting to subscribe again.
 
+When an error is received for a contract with the subscripiton, the module stores the errors in its transient store and executes the Sudo calls at the end block, by reading from the transient store.
+
+## How to use in another module
+
+If any module would like to integrate the x/cwerros functionality, add the following snippet into the `x/<yourmodule>/types/expected_keepers.go` 
+
+```go
+// ErrorsKeeper defines the expected interface needed to interact with the cwerrors module.
+type ErrorsKeeper interface {
+	// SetError records a sudo error for a contract
+	SetError(ctx sdk.Context, sudoErr cwerrortypes.SudoError) error
+}
+```
+
+This keeper would need to be passed in from `app.go` and stored by the module during init. When the module encounters an error it would like to be reported to the x/cwerrors module, it can execute the following snippet.
+
+```go
+sudoerr := cwerrortypes.SudoError {
+		ModuleName:      ModuleName,
+		ErrorCode:       int32(errorCode),
+		ContractAddress: contractAddr,
+		InputPayload:    inputPayload,
+		ErrorMessage:    errMsg,
+}
+err := yourmodulekeeper.errorsKeeper.SetError(ctx, sudoerr)
+```
+
 ## Contents
 
 1. [State](./01_state.md)

x/cwica/ibc_module.go

@@ -39,13 +39,13 @@ func (im IBCModule) OnChanCloseConfirm(ctx sdk.Context, portID string, channelID
 }
 
 // OnAcknowledgementPacket implements the IBCModule interface.
-func (im IBCModule) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, acknowledgement []byte, relayer sdk.AccAddress) error {
-	return im.keeper.HandleAcknowledgement(ctx, packet, acknowledgement, relayer)
+func (im IBCModule) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, acknowledgement []byte, _ sdk.AccAddress) error {
+	return im.keeper.HandleAcknowledgement(ctx, packet, acknowledgement)
 }
 
 // OnTimeoutPacket implements the IBCModule interface.
-func (im IBCModule) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress) error {
-	return im.keeper.HandleTimeout(ctx, packet, relayer)
+func (im IBCModule) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, _ sdk.AccAddress) error {
+	return im.keeper.HandleTimeout(ctx, packet)
 }
 
 // OnChanOpenTry implements the IBCModule interface. We don't need to implement this handler.

x/cwica/keeper/ibc_handlers.go

@@ -55,7 +55,7 @@ func (k *Keeper) HandleChanOpenAck(
 }
 
 // HandleAcknowledgement passes the acknowledgement data to the appropriate contract via a sudo call.
-func (k *Keeper) HandleAcknowledgement(ctx sdk.Context, packet channeltypes.Packet, acknowledgement []byte, relayer sdk.AccAddress) error {
+func (k *Keeper) HandleAcknowledgement(ctx sdk.Context, packet channeltypes.Packet, acknowledgement []byte) error {
 	icaOwner := types.ICAOwnerFromPort(packet.SourcePort)
 	contractAddress, err := sdk.AccAddressFromBech32(icaOwner)
 	if err != nil {
@@ -99,10 +99,10 @@ func (k *Keeper) HandleAcknowledgement(ctx sdk.Context, packet channeltypes.Pack
 	return nil
 }
 
-// HandleTimeout passes the timeout data to the appropriate contract via a sudo call.
+// HandleTimeout creates a new error which can be accessed using the x/cwerrors module.
 // Since all ICA channels are ORDERED, a single timeout shuts down a channel.
 // The channel can be reopened by registering the ICA account again.
-func (k *Keeper) HandleTimeout(ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress) error {
+func (k *Keeper) HandleTimeout(ctx sdk.Context, packet channeltypes.Packet) error {
 	icaOwner := types.ICAOwnerFromPort(packet.SourcePort)
 	contractAddress, err := sdk.AccAddressFromBech32(icaOwner)
 	if err != nil {

x/cwica/keeper/ibc_handlers_test.go

@@ -24,10 +24,9 @@ func (s *KeeperTestSuite) TestHandleAcknowledgement() {
 		contractAddress.String(),
 		contractAdminAcc.Address.String(),
 	)
-	relayerAddress := s.chain.GetAccount(1).Address
 
 	// TEST CASE 1: invalid contract address
-	err := cwicaKeeper.HandleAcknowledgement(ctx, channeltypes.Packet{}, nil, relayerAddress)
+	err := cwicaKeeper.HandleAcknowledgement(ctx, channeltypes.Packet{}, nil)
 	s.Require().ErrorContains(err, "failed to parse contract address: : invalid address")
 
 	// TEST CASE 2: invalid packet acknowledgement
@@ -36,7 +35,7 @@ func (s *KeeperTestSuite) TestHandleAcknowledgement() {
 		SourcePort:    icatypes.ControllerPortPrefix + contractAddress.String(),
 		SourceChannel: "channel-0",
 	}
-	err = cwicaKeeper.HandleAcknowledgement(ctx, p, nil, relayerAddress)
+	err = cwicaKeeper.HandleAcknowledgement(ctx, p, nil)
 	s.Require().ErrorContains(err, "cannot unmarshal ICS-27 packet acknowledgement")
 
 	// TEST CASE 3: success contract SudoResponse
@@ -46,12 +45,12 @@ func (s *KeeperTestSuite) TestHandleAcknowledgement() {
 	resAckData, err := channeltypes.SubModuleCdc.MarshalJSON(&resACK)
 	s.Require().NoError(err)
 
-	err = cwicaKeeper.HandleAcknowledgement(ctx, p, resAckData, relayerAddress)
+	err = cwicaKeeper.HandleAcknowledgement(ctx, p, resAckData)
 	s.Require().NoError(err)
 
 	// TEST CASE 4: contract callback fails - should not return error - because error is swallowed
 	wmKeeper.SetReturnSudoError(errors.New("error sudoResponse"))
-	err = cwicaKeeper.HandleAcknowledgement(ctx, p, resAckData, relayerAddress)
+	err = cwicaKeeper.HandleAcknowledgement(ctx, p, resAckData)
 	s.Require().NoError(err)
 }
 
@@ -71,10 +70,9 @@ func (s *KeeperTestSuite) TestHandleTimeout() {
 		contractAddress.String(),
 		contractAdminAcc.Address.String(),
 	)
-	relayerAddress := s.chain.GetAccount(1).Address
 
 	// TEST CASE 1: invalid contract address
-	err := cwicaKeeper.HandleTimeout(ctx, channeltypes.Packet{}, relayerAddress)
+	err := cwicaKeeper.HandleTimeout(ctx, channeltypes.Packet{})
 	s.Require().ErrorContains(err, "failed to parse contract address: : invalid address")
 
 	// TEST CASE 2: success contract SudoResponse
@@ -84,12 +82,12 @@ func (s *KeeperTestSuite) TestHandleTimeout() {
 		SourceChannel: "channel-0",
 	}
 
-	err = cwicaKeeper.HandleTimeout(ctx, p, relayerAddress)
+	err = cwicaKeeper.HandleTimeout(ctx, p)
 	s.Require().NoError(err)
 
 	// TEST CASE 3: contract callback fails - should not return error - because error is swallowed
 	wmKeeper.SetReturnSudoError(errors.New("SudoTimeout error"))
-	err = cwicaKeeper.HandleTimeout(ctx, p, relayerAddress)
+	err = cwicaKeeper.HandleTimeout(ctx, p)
 	s.Require().NoError(err)
 }
 

x/cwica/spec/02_messages.md

@@ -75,7 +75,7 @@ message MsgSendTx {
 ```
 
 On Success
-* An ibc packet is created which will be executed on the counterparty chain
+* An ibc packet is created will need to be picked up by the relayer
 
 This mesasge is expected to fail if:
 * There are no msgs to be executed

x/cwica/spec/06_ibc.md

@@ -0,0 +1,48 @@
+# IBC Handlers
+
+Section describes the processing of the various IBC events
+
+## OnChanOpenInit
+
+There is no custom logic executed under this handler.
+
+## OnChanOpenTry
+
+This handler is not implemented for controller, only host.
+
+## OnChanOpenAck
+
+This handler is executed when a channel is successfully created between the chain and the counterparty chain. 
+
+On success ack: 
+* The handler parses the counterparty version details and extracts the counterparty address and executes a Sudo call to the contract with the details
+
+## OnChanOpenConfirm
+
+This handler is not implemented for controller, only host.
+
+## OnChanCloseInit
+
+This handler is not implemented for controller, only host.
+
+## OnChanCloseConfirm
+
+This handler is currently not implemented by the module
+
+## OnRecvPacket
+
+This handler is not implemented for controller, only host.
+
+## OnAcknowledgementPacket
+
+This handler is executed when an ibc acknowledgement packet is received for a MsgSendTx operation.
+
+On success:
+* The handler formats the ibc packet and acknowledgement result into a sudo payload and executes a Sudo call to the contract with the details.
+
+On failure:
+* The handler formats the ibc packet and error into a Sudo Error with error code [ERR_EXEC_FAILURE](05_errors.md) and sends it to the x/cwerrors module
+
+## OnTimeoutPacket
+
+This handler is executed when an ibc packet times out. It formats the ibc packet int oa Sudo Error with error code  [ERR_PACKET_TIMEOUT](05_errors.md) and sends it to the x/cwerrors module

x/cwica/spec/README.md

@@ -66,7 +66,27 @@ let sendtx_stargate_msg = CosmosMsg::Stargate {
 Ok(Response::new().add_message(sendtx_stargate_msg))
 ```
 
-Once the txs have been submitted, the contract will receive a callback at the Sudo entrypoints. [Here is how to integrate them](../../../proto/archway/cwica/v1/sudo.proto)
+Once the txs have been submitted and is successfully executed on the counterparty chain, the contract will receive a callback at the Sudo entrypoints. It can be integrated by implementing the snippet below
+
+```rust
+// msg.rs
+pub enum SudoMsg  {
+    Ica {
+        account_registered: Option<AccountRegistered>,
+        tx_executed: Option<ICAResponse>,
+    },
+}
+
+#[cw_serde]
+pub struct AccountRegistered {
+    pub counterparty_address: String,
+}
+
+#[cw_serde]
+pub struct ICAResponse {
+    pub data: Binary,
+}
+```
 
 
 > **NOTE** 
@@ -79,4 +99,5 @@ Once the txs have been submitted, the contract will receive a callback at the Su
 2. [Messages](./02_messages.md)
 3. [Client](./03_client.md)
 4. [Wasm Bindings](./04_wasm_bindings.md)
-5. [Module Errors](./05_errors.md)
+5. [Module Errors](./05_errors.md)
+6. [IBC Handlers](./06_ibc.md)

x/cwica/types/errors.go

@@ -8,8 +8,6 @@ import (
 
 // x/cwica module sentinel errors
 var (
-	ErrInvalidAccountAddress            = errors.Register(ModuleName, 1101, "invalid account address")
-	ErrInterchainAccountNotFound        = errors.Register(ModuleName, 1102, "interchain account not found")
 	ErrNotContract                      = errors.Register(ModuleName, 1103, "not a contract")
 	ErrEmptyConnectionID                = errors.Register(ModuleName, 1104, "empty connection id")
 	ErrCounterpartyConnectionNotFoundID = errors.Register(ModuleName, 1105, "counterparty connection id not found")