# SIGN_MODE_TEXTUAL Internal Audit Report

Date: March-April 2023
Auditors: Amaury (https://github.com/amaurym), Facundo (https://github.com/facundomedica)

## Introduction

SIGN_MODE_TEXTUAL is a new string-based sign mode that is targetted at signing with hardware devices. It is specified in [ADR-050](https://github.com/cosmos/cosmos-sdk/blob/main/docs/architecture/adr-050-sign-mode-textual.md).

In 2022-2023, the Cosmos SDK team led a working group with Agoric, Confio, StrangeLove and Zondax to implement SIGN_MODE_TEXTUAL. As part of its Quality Assurance process, this working group is conducting an internal audit to identify potential security issues, inefficiencies and non-compliance to the specification.

This document describes the results of the internal audit.

## Executive Summary

Seven findings were identified and categorized into Specification, Implementation, Testing, and Documentation. Their severities ranged from Informational to Medium, no critical issues were found. The most critical issue was the lack of a boolean value renderer which would prevent any message including a bool value from being signed using sign mode textual. The findings where addressed in PRs or tracked in a GitHub issue.

## Scope

The internal audit is performed on the https://github.com/cosmos/cosmos-sdk repository, at commit hash `a8dceddad91cf0b00b20c4f7d4260583719a6403` (March 20th, 2023).

The scope limited to the following files and folders:
- [ ] Specification
- [ ] Main `docs/architecture/adr-050-sign-mode-textual.md`
- [ ] Annex 1`docs/architecture/adr-050-sign-mode-textual-annex1.md`
- [ ] Annex 2`docs/architecture/adr-050-sign-mode-textual-annex2.md`
- [ ] Value Renderers (`x/tx/signing/textual/*`)
- [ ] any
- [ ] bytes
- [ ] coin/coins
- [ ] dec
- [ ] duration
- [ ] enum
- [ ] int
- [ ] message
- [ ] repeated
- [ ] string
- [ ] timestamp
- [ ] tx envelope `x/tx/signing/textual/tx.go`
- [ ] dispatcher `x/tx/signing/textual/handler.go`
- [ ] Encoding
- [ ] CBOR implementation `x/tx/signing/textual/internal/cbor`
- [ ] Sign Doc `x/tx/signing/textual/encode.go`
- [ ] Wiring with the SDK
- [ ] sign mode handler `x/auth/tx/textual.go`
- [ ] server-side instantiation `x/auth/tx/config.NewTextualWithBankKeeper`
- [ ] client-side instantiation `x/auth/tx/config.NewTextualWithGRPCConn`

## Methodology

The methodology is based on the "Module Readiness Checklist" by the Cosmos SDK, which has been used as an internal audit template when introducing new SDK modules (e.g. authz, feegrant, groups). The template can be found [in the Cosmos SDK repository](https://github.com/cosmos/cosmos-sdk/blob/v0.47.1/.github/ISSUE_TEMPLATE/module-readiness-checklist.md).

It has been slightly adapted, as SIGN_MODE_TEXTUAL is not an SDK module.

The following checklist should be run for each item in the Scope.

- [ ] API audit
- [ ] Are public structs, interfaces, methods and types well-named and organized?
- [ ] Is everything well documented (inline godoc)?
- [ ] Code correctness
- [ ] Verify correctness upon visual inspection
- [ ] Ensure all state machine code which could be confusing is properly commented
- [ ] Ensure that all state machine edge cases are covered with tests and that test coverage is sufficient
- [ ] 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
- [ ] Assess potential risks of any new third party dependencies and decide whether a dependency audit is needed
- [ ] Spec
- [ ] Can we improve human readability while maintaining security?
- [ ] Is the spec fully implemented?
- [ ] Are there implementation choices that should be documented in the spec?

## Findings

|ID|Title|Category|Severity|Status|
|--|--|--|--|--|
|01-ANY-SPEC|`Any` value renderer spec update|Documentation|Low|Solved|
|02-COIN-TEST|`Coin` value renderer "zero" test case|Testing|Low|Solved|
|03-MSG-TEST|`Message` value renderer spec update|Documentation|Informational|Solved|
|04-ANY-HEADER|`Any` value renderer header mismatch|Implementation|Low|Solved|
|05-MSG-DUPCODE|`Message` value renderer duplicated code|Implementation|Low|Solved|
|06-BOOL-RDR|Bool value renderer missing|Specification|Medium|Solved|
|07-OFFLINE-SIGN|Offline signing is not possible|Implementation|Medium|TO-DO, not planned|

Finding Category
- Specification: A flaw or problem in the specification or protocol.
- Implementation: A problem with the source code. For example a bug, a divergence from a specification, or a poor choice of data structure.
- Testing: A lack of test coverage on a critical code path.
- Code structure: A problem that impacts the extent to which the project is maintainable and understandable by developers in the long term.
- Documentation: A lack of documentation, or insufficient clarity, accuracy or readability of existing documentation.

Finding Severity:
- Informational: The issue does not pose an immediate risk (it is subjective in nature). Findings with Informational severity are typically suggestions around best practices or readability.
- Low: The issue is objective in nature, but the security risk is relatively small or does not represent a security vulnerability.
- Medium: The issue is a security vulnerability that may not be directly exploitable or may require certain complex conditions in order to be exploited.
- High: The issue is an exploitable security vulnerability.

### 1. `Any` value renderer spec update

ID: 01-ANY-SPEC
Category: Documentation
Severity: Low
Status: Solved in https://github.com/cosmos/cosmos-sdk/pull/15550

Artifacts: Annex 1.

Description: As discussed during a Working Group call, it was decided to remove the header screen for messages. The implementation was done https://github.com/cosmos/cosmos-sdk/pull/15208, however the specification was not updated accordingly.

Recommendation: Update the specification to match the agreed upon decision and the implementation. Also add in-code comments to reference this spec.

### 2. `Coin` value renderer "zero" test case

ID: 02-COIN-TEST
Category: Testing
Severity: Low
Status: Solved in https://github.com/cosmos/cosmos-sdk/pull/15550

Artifacts: `x/tx/signing/textual/coins.go`

Description: The specification describes empty coin encoded as `"zero"`. The `x/tx/signing/textual/internal/testdata/coin.json` however did not contain a test case for this.

Recommendation: Add the test case for `"zero"`.

### 3. `Message` value renderer spec update

ID: 03-MSG-SPEC
Category: Documentation
Severity: Low
Status: Solved in https://github.com/cosmos/cosmos-sdk/pull/15550

Artifacts: Annex 1.

Description: The specification describes a special case for `sdk.Msg` message, which was then abandoned in a Working Group call.

Recommendation: Remove the special case for `sdk.Msg`, clarify specification more.

### 4. `Any` value renderer header mismatch

ID: 04-ANY-HEADER
Category: Implementation
Severity: Low
Status: Solved in https://github.com/cosmos/cosmos-sdk/pull/15550

Artifacts: `x/tx/signing/textual/any.go`

Description: The `Any` value-renderer contains the [code](https://github.com/cosmos/cosmos-sdk/blob/a8dceddad91cf0b00b20c4f7d4260583719a6403/x/tx/signing/textual/any.go#L54-L55):

    _, isMsgRenderer := vr.(*messageValueRenderer)
    if isMsgRenderer && subscreens[0].Content == fmt.Sprintf("%s object", internalMsg.ProtoReflect().Descriptor().Name()) {

If the first condition `isMsgRenderer` is true, then the second condition also must be true, or else it's an implementation error upstream (in the protobuf values passed into Textual), and the user should be aware of it.

Recommendation: Throw an error if the second condition doesn't pass.

    _, isMsgRenderer := vr.(*messageValueRenderer)
-    if isMsgRenderer && subscreens[0].Content == fmt.Sprintf("%s object", internalMsg.ProtoReflect().Descriptor().Name()) {
+    if isMsgRenderer {
+       if subscreens[0].Content != fmt.Sprintf("%s object", internalMsg.ProtoReflect().Descriptor().Name()) {
+           return nil, fmt.Errorf("any internal message expects %s, got %s", fmt.Sprintf("%s object", internalMsg.ProtoReflect().Descriptor().Name()), subscreens[0].Content)
+       }
}

### 5. `Message` value renderer duplicated code

ID: 05-MSG-DUPCODE
Category: Implementation
Severity: Low
Status: Solved in https://github.com/cosmos/cosmos-sdk/pull/15715

Artifacts: `x/tx/signing/textual/message.go`

Description: The `Message` value renderer made use of a hardcoded suffix in many places instead of using an already existing method for this (instead of `fmt.Sprintf("%s object", ...)` use `header()`).

### 6. Bool value renderer missing

ID: 06-BOOL-RDR
Category: Implementation/Spec
Severity: Medium/High
Status: Solved in https://github.com/cosmos/cosmos-sdk/pull/15863

Description: There is no bool value renderer, meaning that any message that contains a bool can't be signed and will throw an error (for example x/gov proposal's expedited field is a boolean).

### 7. Offline signing is not possible

ID: 07-OFFLINE-SIGN
Category: Implementation
Severity: Low
Status: Tracked in https://github.com/cosmos/cosmos-sdk/issues/15864

Description: Sign mode textual requires a GRPC connection or a bank keeper to query a denom's metadata in order to display it correctly; none of which will/should be available if --offline is being passed.