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.

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

Check for token minting or burning #692

Merged
merged 16 commits into from
Feb 3, 2023

Conversation

ffakenz
Copy link
Contributor

@ffakenz ffakenz commented Feb 1, 2023

Why

We want to close the identified security gap where we want to prevent minting or burning of tokens in state transitions that are not actually supposed to burn or mint.

How

🍦 Update checkClose, checkContest, checkCommit and checkCollectCom to verify no head token is burnt in v-head.

To check before merging:

  • CHANGELOG is up to date
  • Documentation is up to date

@github-actions
Copy link

github-actions bot commented Feb 1, 2023

Transactions Costs

Sizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using arbitrary values and results are not fully deterministic and comparable to previous runs.

Metadata
Generated at 2023-02-03 12:20:40.514204503 UTC
Max. memory units 14000000
Max. CPU units 10000000000
Max. tx size (kB) 16384

Script summary

Name Hash Size (Bytes)
νInitial 31eac6a7270af7736ee56d0447e7c8512a54ef8aafd67da5e33f3b2f 5530
νCommit 08bb32edf316341c4fa7b0e3ed0e77bccb7f1d3b687c10d1b1e48969 2961
νHead 492a5736cefa8ec8dd72cbfef83da238c711f2a8c1b1f07d33b3678e 9969

Cost of Init Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 4984 10.47 4.15 0.48
2 5188 12.31 4.85 0.51
3 5392 13.88 5.44 0.54
5 5803 19.11 7.50 0.61
10 6829 27.72 10.78 0.75
46 14210 99.55 38.47 1.86

Cost of Commit Transaction

Currently only one UTxO per commit allowed (this is about to change soon)

UTxO Tx size % max Mem % max CPU Min fee ₳
1 633 21.10 8.53 0.41

Cost of CollectCom Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 13738 24.80 9.71 1.03
2 14090 44.14 17.41 1.25
3 14263 62.49 24.82 1.46
4 14684 91.60 36.45 1.80

Cost of Close Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 10602 15.57 6.09 0.79
2 10764 16.90 6.74 0.81
3 10899 17.84 7.24 0.83
5 11225 20.49 8.53 0.87
10 12117 27.81 12.04 0.99
30 15416 54.17 24.92 1.45
59 16347 67.74 25.09 1.60

Cost of Contest Transaction

Parties Tx size % max Mem % max CPU Min fee ₳
1 10554 15.01 5.87 0.78
2 10726 16.08 6.42 0.80
3 10891 17.66 7.16 0.82
5 11253 20.38 8.49 0.87
10 12148 27.74 12.01 1.00
30 15415 53.94 24.83 1.44
35 16249 61.06 28.26 1.56

Cost of Abort Transaction

Some variation because of random mixture of still initial and already committed outputs.

Parties Tx size % max Mem % max CPU Min fee ₳
1 15064 28.59 12.07 1.13
2 14962 37.14 15.42 1.22
3 15703 65.75 28.15 1.58
4 15739 83.92 35.91 1.78

Cost of FanOut Transaction

Involves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.

UTxO Tx size % max Mem % max CPU Min fee ₳
1 14882 11.01 4.75 0.93
2 14913 12.24 5.51 0.95
3 15017 14.14 6.54 0.97
5 14896 16.05 7.86 0.99
10 15136 24.05 12.41 1.10
20 15432 38.91 21.06 1.30
46 16369 78.57 43.98 1.82

@v0d1ch v0d1ch requested review from ch1bo, pgrange and a user February 1, 2023 16:19
Copy link
Contributor

@pgrange pgrange left a comment

Choose a reason for hiding this comment

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

This review tries to apply the perfection game protocol

I will describe what I like about this P.R. and then what improvement I can think of to make it perfect. I'm expecting the author of the P.R. to ensure that they understand what I write or ask for clarifications if it's not the case. I'm expecting the author of the P.R. to just not include my ideas if they disagree with them without any form of justification. Hence I, in advance, approve the P.R.

To describe what I would change, I will try to extensively use the suggest change feature of GitHub as I believe this is the most efficient way to upload data from my brain to the authors' brains. That being said, the suggestions I will write will probably not compile or pass the test so do not just accept them but consider them for what they are: the description of an improvement to make the P.R. perfect.

What I like about this P.R.:

  • it's been done in a very short time with a very small and precise scope in mind
  • I like the name chosen for the new added functions which are explicit and clear to me

For me to find it perfect you would have to:

  • make the tests pass
  • polish the commit history to have only one update changelog commit
  • polish the commit history to have only one haddock commit
  • polish the commit history to include the changes of the one named minor into another commit
  • detail the P.R. description adding at least a why section which would explain why we need to check that in the validator, in particular, explain how one could forge a transaction which would burn token in a close transaction
  • see other improvement proposals below

hydra-node/test/Hydra/Chain/Direct/Contract/Close.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/Contest.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/CollectCom.hs Outdated Show resolved Hide resolved
@v0d1ch v0d1ch force-pushed the ensemble/check-for-token-burning-in-v-head branch 2 times, most recently from 0dfed0e to 0d67991 Compare February 1, 2023 17:05
@ch1bo ch1bo linked an issue Feb 2, 2023 that may be closed by this pull request
Copy link
Collaborator

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

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

Commit transaction also must not mint/burn tokens. Also we want to prevent any minting or burning (per the specification).

CHANGELOG.md Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/Contest.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/Close.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/Mutation.hs Outdated Show resolved Hide resolved
hydra-plutus/src/Hydra/Contract/Head.hs Outdated Show resolved Hide resolved
hydra-plutus/src/Hydra/Contract/Head.hs Outdated Show resolved Hide resolved
@v0d1ch v0d1ch force-pushed the ensemble/check-for-token-burning-in-v-head branch from 51e958d to 71e9968 Compare February 2, 2023 12:07
@v0d1ch v0d1ch changed the title Check for no head token burning in v head Check for token minting or burning Feb 2, 2023
@v0d1ch v0d1ch force-pushed the ensemble/check-for-token-burning-in-v-head branch from 58d5d55 to 67e6f2d Compare February 2, 2023 14:49
@v0d1ch v0d1ch requested review from ch1bo and pgrange February 2, 2023 14:50
v0d1ch and others added 11 commits February 2, 2023 15:44
Values are a bit weird to work with because "no minting" actually yields
a Value with the ada symbol and quantity 0 always. This is due to the
way the ledger converts babbage tx bodies to plutusv2 script contexts.
- Rename Mutation constructors MutateTokenBurning -> MutateTokenMintingOrBurning
- Move mustNotMintOrBurn to the Util module
- Fix haddock strings to reflect also minting
- Move changelog entry and fix wording
- Fix haddock to not include the word _forge_
@ffakenz ffakenz force-pushed the ensemble/check-for-token-burning-in-v-head branch from 67e6f2d to f9eb387 Compare February 2, 2023 18:52
Copy link
Collaborator

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

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

The mint/burn generator is good enough. Location could be improved. But address the MUST comments. Also, tests don't pass anymore as the script is now 100Bytes bigger again.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
hydra-node/hydra-node.cabal Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/Mutation.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/Chain/Direct/Contract/Mutation.hs Outdated Show resolved Hide resolved
hydra-node/hydra-node.cabal Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Feb 3, 2023

Test Results

271 tests   - 11   265 ✔️  - 11   17m 8s ⏱️ + 3m 43s
  92 suites  -   4       6 💤 ±  0 
    4 files    -   1       0 ±  0 

Results for commit c357f37. ± Comparison against base commit b637137.

This pull request removes 11 tests.
Hydra.TUI.Options ‑ parses --cardano-signing-key option
Hydra.TUI.Options ‑ parses --connect option
Hydra.TUI.Options ‑ parses --network-id option
Hydra.TUI.Options ‑ parses --node-socket option
Hydra.TUI/end-to-end smoke tests ‑ display feedback long enough
Hydra.TUI/end-to-end smoke tests ‑ doesn't allow multiple initializations
Hydra.TUI/end-to-end smoke tests ‑ starts & renders
Hydra.TUI/end-to-end smoke tests ‑ supports the full Head life cycle
Hydra.TUI/end-to-end smoke tests ‑ supports the init & abort Head life cycle
Hydra.TUI/text rendering errors ‑ should show not enough fuel message and suggestion
…

♻️ This comment has been updated with latest results.

@v0d1ch v0d1ch force-pushed the ensemble/check-for-token-burning-in-v-head branch from 08d39fd to c357f37 Compare February 3, 2023 12:10
@v0d1ch v0d1ch merged commit ef72603 into master Feb 3, 2023
@v0d1ch v0d1ch deleted the ensemble/check-for-token-burning-in-v-head branch February 3, 2023 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check for no token burning needs to be a part of v_head checks
4 participants