Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CIP-0111? | Web-Wallet Bridge - Wallet Transaction Caching #733

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

CIP-0111? | Web-Wallet Bridge - Wallet Transaction Caching #733

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b93778a
feat: Wallet Transaction Caching CIP
yHSJ Dec 22, 2023
197606c
original PR is the most important Discussion
rphair Jan 5, 2024
e3e8e10
Path to Active going forward from this point indicates this is only P…
rphair Jan 5, 2024
a6cf40d
no implementors yet, but they are expected in future
rphair Jan 5, 2024
f5d9c58
H1 title is automatically generated: not put in markup
rphair Jan 5, 2024
0d7d3ea
copyright must exist in markup, not just as an unused link
rphair Jan 5, 2024
b191b27
assigned CIP number 111
rphair Jan 23, 2024
ff5e999
chore: fix spelling mistakes
yHSJ Jan 23, 2024
d730535
Update cip-smart-tx-signing/README.md
yHSJ Feb 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions cip-smart-tx-signing/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
---
CIP: ?
rphair marked this conversation as resolved.
Show resolved Hide resolved
Title: Wallet Transaction Caching
Status: Proposed
Category: Wallets
Authors:
- Josh Marchand <josh@securitybot.info>
Implementors: []
Discussions:
- https://github.com/cardano-foundation/CIPs/pull/733
Created: 2023-12-22
License: CC-BY-4.0
---

## Abstract

This CIP extends [CIP-0030 Cardano dApp-Wallet Web Bridge](https://github.com/cardano-foundation/CIPs/tree/master/CIP-0030/) and [CIP-0103? | Web-Wallet Bridge - Bulk Transaction Signing](https://github.com/cardano-foundation/CIPs/pull/587) to allow wallets to provide a complete and accurate summary of transactions that spend outputs that don't exist on chain yet when possible, and clearly communicate to the user risks when they cannot.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change this from being an explicit extension to these CIPs and rather be a best practice that applies to these other CIPs

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree in general, I was just trying to follow the standards for "adding on" to the existing CIPs. That said, I think that it makes more sense to not explicitly extend any one CIP, especially as this CIP should be used along side ANY CIP that involves a wallet prompting users for a signature.


## Motivation: why is this CIP necessary?

Transaction chaining, building transactions that spend outputs of transactions that aren't settled on chain, allows dApps to quickly do several on-chain transactions instead of waiting for each indvidual transaction to settle before building and submitting the next one. This is a powerful feature that has allowed dApps to provide seamless experiences to users. However, due to the tranasaction data structure, wallets are unable to provide a complete and accurate summary of these chained transactions when prompting users for a signature.
yHSJ marked this conversation as resolved.
Show resolved Hide resolved

A transaction input, which is a reference to a previous transaction output, is defined in the [babbage CDDL](https://github.com/IntersectMBO/cardano-ledger/blob/master/eras/babbage/impl/cddl-files/babbage.cddl) as follows:

```
transaction_input = [ transaction_id : $hash32
, index : uint
]
```

Therefore, in order for a wallet to know what assets are being spent by a transaction, it must have access to the transaction that the input is referencing. This is not a problem when the transaction inputs reference transactions that have settled on chain. However, when a transaction is being signed that spends outputs that don't exist on chain yet, the wallet cannot know what the spent UTxOs actually contain.

A malicious actor could take advantage of this by creating two transactions that are chained together. The first transaction would be a transaction that spends a user's UTxOs and creates new UTxOs that are locked by the user's address. The second transaction would then spend these new UTxOs and send the funds to the attacker's address. The malicious actor would then prompt the user to sign both transactions, using [CIP 30's signTx](https://github.com/cardano-foundation/CIPs/blob/master/CIP-0030/README.md#apisigntxtx-cbortransaction-partialsign-bool--false-promisecbortransaction_witness_set) endpoint. The first transaction summary would show nothing malicious, and the user would thus sign it. The second transaction would show an incorrect or incomplete transaction summary, and because user's are used to seeing those summaries from other dApps, they may sign the second transaction, effectively sending all their assets to the attacker.

Simply caching signed tranasctions before they appear on chain and refusing to sign transactions that spend outputs that don't exist on chain yet would solve this problem. However, this would break some transaction chaining done by dApps, where they reference UTxOs that were created in a transaction that didn't involve the user but hasn't settled yet. Therefore, the best solution would be to cache signed transactions and display a clear warning to the user when a transaction includes inputs that reference outputs that can't be found.

## Specification

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this document are to be interpreted as described in [RFC 2119](https://datatracker.ietf.org/doc/html/rfc2119).

### Defintions

**Wallet**: A wallet is any software that manages a user's private keys and provides basic cryptographic operations such as signing data, for the purpose of interacting with the Cardano blockchain.

---

A wallet MUST maintain a cache of signed transactions that have not yet settled on chain. This cache MAY have a maximum size, and if the cache is full, the wallet MAY remove transactions from the cache, beginning with the oldest transaction to store a newer transaction. The wallet MAY remove cached transactions once they are seen on chain.

If a user signs a transaction through the [CIP-30 signTx](https://github.com/cardano-foundation/CIPs/blob/master/CIP-0030/README.md#apisigntxtx-cbortransaction-partialsign-bool--false-promisecbortransaction_witness_set) endpoint, the [CIP-103? signTxs](https://github.com/cardano-foundation/CIPs/blob/508ea0557bcd17d793da90312789165dcef8a4db/CIP-0103/README.md#apicip103signtxstxs-transactionsignaturerequest-promisecbortransaction_witness_set) endpoint, or any other CIP-30 extension that allows signing transactions, the wallet MUST cache the signed transaction with a TTL (time-to-live) greater than or equal to the [TTL of the transaction](https://github.com/IntersectMBO/cardano-ledger/blob/master/eras/babbage/impl/cddl-files/babbage.cddl#L57C8-L57C9). The wallet MUST NOT remove the transaction from the cache until the TTL has expired, the transaction has been seen on chain, or the cache is full and the wallet needs to remove transactions to make room for newer transactions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think cache size is a concern here. The cache will only contain utxos owned by the wallet, so even if we consider ttl, it is safe to assume that the size will not grow beyond manageable values, as each utxo creation will incur costs. We already implicitly allow the wallet size to grow indefinitely (because the number of utxos in a multi-address wallet is unlimited).

If we allow valid implementations to drop cache data due to size limits, then a dApp would not be able to rely on presence of any data in the cache. And an implementation with SIZE=0 will be a valid one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this somehow. I think that it's really important to define this behavior in a standard. Lots of the current CIPs have undefined behaviors in edge cases and it can cause real issues across the ecosystem.

I want this standard to be comprehensive, regardless of device or wallet software. In the (rare, as you correctly point out) case that a cache has to be purged, it is important the wallet purges transactions in an appropriate manner (ascending chronologically).

It's an excellent point that a wallet could correctly implement this CIP with a cache size of zero bytes. I think it makes sense to add a "minimum cache size" that is n * maxTxSize. I would love to hear others' thoughts on what the value of n should be, since it will necessarily add (small) overhead to the wallet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n * maxTxSize

Why do we need to cache whole transactions? I think it could work with caching just UTxOs, and only those that belong to one of the user's addresses.

Copy link
Author

@yHSJ yHSJ Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the specific problem in the CIP is solved with just the UTxOs, I felt it may be useful to have the complete transaction. For example, caching the inputs as well would allow a wallet to refuse a signature on a transaction which consumes inputs which are already consumed in a cached transaction. Perhaps that is out of the scope of this CIP in particular though.


When displaying a transaction summary to the user, the wallet MUST search for the outputs referenced by inputs in the following order:

1. The latest known on-chain UTxO set
2. The cache of signed transactions
3. The set of transactions being signed

If a transaction contains an input that references an output that cannot be found, the wallet MUST display a warning to the user that the transaction includes unidentifiable inputs. The style and wording of this warning is left up to the wallet. The wallet MAY prevent the user from signing a transaction with unidentified inputs, but this may cause issues when interacting with some dApps.
yHSJ marked this conversation as resolved.
Show resolved Hide resolved

## Rationale: how does this CIP achieve its goals?

Copy link
Collaborator

@Ryun1 Ryun1 Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is worth discussing here the alternatives to this approach, such as stronger warnings within wallets OR not letting users sign unknown transactions.

The proposed approach allows wallets to provide a complete and accurate summary of transactions without prohibiting transaction chaining or requiring dApps to change the way they interface with wallets.

Although it cannot entirely reduce the risk of a malicious actor tricking a user into signing a transaction that they didn't intend to, it will hopefully reduce the risk by showing an accurate summary of the transaction more often and, when not possible, displaying a clear warning to the user.

## Path to Active

### Acceptance Criteria

In order for this standard to be considered 'Active', the following criteria must be met:

- [ ] A thorough review by all relevant stakeholders
- [ ] Implementation in at least two Cardano wallets

### Implementation Plan

N/A
rphair marked this conversation as resolved.
Show resolved Hide resolved

## Copyright

This CIP is licensed under [CC-BY-4.0](https://creativecommons.org/licenses/by/4.0/legalcode).