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.

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:

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.

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.

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:


    _, 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.

Syntax	Example	Reference
# Header	Header	基本排版
- Unordered List	Unordered List
1. Ordered List	Ordered List
- [ ] Todo List	Todo List
> Blockquote	Blockquote
Bold font	Bold font
Italics font	Italics font
~~Strikethrough~~	~~Strikethrough~~
19^th^	19^th
H~2~O	H₂O
++Inserted text++	Inserted text
==Marked text==	Marked text
[link text](https:// "title")	Link
![image alt](https:// "title")	Image
`Code`	`Code`	在筆記中貼入程式碼
```javascript var i = 0; ```	`var i = 0;`	在筆記中貼入程式碼
:smile:		Emoji list
{%youtube youtube_id %}	Externals
$L^aT_eX$	L^aT_eX
:::info This is a alert area. :::	This is a alert area.

SIGN_MODE_TEXTUAL Internal Audit Report

Introduction

Executive Summary

Scope

Methodology

Findings

1. Any value renderer spec update

2. Coin value renderer "zero" test case

3. Message value renderer spec update

4. Any value renderer header mismatch

5. Message value renderer duplicated code

6. Bool value renderer missing

7. Offline signing is not possible

1. `Any` value renderer spec update

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

3. `Message` value renderer spec update

4. `Any` value renderer header mismatch

5. `Message` value renderer duplicated code