docs: add initial threat model diagrams for multipool autoswap and treasury #3888

jessysaurusrex · 2021-09-24T06:17:00Z

Introducing the initial threat model diagrams for the MultiPool AutoSwap and Treasury.

katelynsills · 2021-09-24T17:50:59Z

This is awesome! Feel free to add me as a reviewer when you're ready and we can get this merged in!

jessysaurusrex · 2021-09-24T19:50:41Z

This draft set is ready to go -- the main goal is to make them available for others to engage with. Generally, I expect this folder to grow incrementally as more diagrams are ready. It's likely that the the entire effort will be be as "done" as can be considered the accompanying written documentation around risk/mitigation/remediation is merged.

katelynsills

This looks great! Some small comments

katelynsills · 2021-09-27T16:25:22Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_component.puml

+    package "publicFacet" as pf0 {
+        interface "addPool()" as ap
+        interface "otherMethods..." as om
+        interface "offer()" as offer


I think offer should be part of Zoe instead. Is there a reason to put it on the publicFacet?

Technically, yes -- but in order to simplify the diagram and not bring the entirety of Zoe into this context, I opted to represent calling Zoe through the publicFacet. We should (and will) diagram all Zoe APIs in a separate diagram pending ongoing work, but this isn't the right place to do that just yet.

Ok, it makes sense to me to leave out Zoe as an object in the diagram for simplification's sake. But, the diagram is going to be misleading in important ways if offer is on the public facet of the contract. The viewer would think that contract code handles offer payments, which could violate offer safety and payout liveness, thus undermining Zoe entirely.

So, this is something we need to change before this gets merged in. But, I think there's an easy fix, which is an asterisk + comment on the diagram saying that offer is actually part of Zoe, and no contract code has access to the user's payments.

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

katelynsills · 2021-09-27T16:31:44Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

+group recovering funds
+pu -> usA: ""getPayout("in")""
+return ""payoutIn""
+pu -> pu: redeposit ""payoutIn"" into purse
+pu -> usA: ""getPayout("out")""
+return ""payoutOut""
+pu -> pu: redeposit ""payoutOut"" into purse
+end


When adding liquidity, the user's offer will have the keywords "Central", "Secondary", and "Liquidity". I think these are the keywords from the swapping offers.

Also a nitpick, but rather than "redeposit" I think we should say "deposit" because there might have been no previous deposit, and never any previous deposit of the same payment.

The details of the offer record are excluded here in order to simplify the diagram. In and Out should encompass the resources from the completed offer result.

Initially, I was thinking "redeposit" as I wanted to capture the possibility that this may actually have been initial funds. Any ideas on how to capture that in addition to "deposit"? Failing to deposit these funds would result in their loss, and that would need to be captured somehow.

I think you should use the term deposit. Redeposit isn't going to be accurate in most cases, especially since the contract can give you more than what you gave. Plus it's shorter :)

About "In" and "Out", I think there's a misunderstanding. What I'm advocating for shouldn't complicate the diagram much. I'm saying replace the word "Out" with "Liquidity", replace the word "In" with "Central", and add a line for an additional payment for "Secondary". Is there any reason not to do that? I think there's no need to generalize the payments since we know what they are, and it's pretty simple to replace the words.

Updated redeposit to deposit.

I see what you mean regarding In/Out - I updated the terms to reflect what was in the original offer.

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

katelynsills · 2021-09-27T16:42:19Z

docs/threat_models/treasury/treasury.puml

+    circle getPayoff
+    circle getPayoffs


Is there a reason to include both of these?

Both of these methods appear in the Treasury contract, and should be represented independently. (If they aren't in use, it would be worth considering eliminating one for the sake of simplicity.)

I just searched and I couldn't find anything that looked like getPayoff or getPayoffs. Where in the contract are you thinking of?

Oops I got my wires crossed, s/Payoff/Payout/g. These both appear to be callable in the contract.

docs/threat_models/treasury/treasury.puml

…easury

katelynsills · 2021-09-28T03:19:11Z

I've already approved, but adding @Chris-Hibbert in case there is further discussion/questions. Thanks @jessysaurusrex and @Chris-Hibbert!

docs/threat_models/smart_contracts/multipool_autoswap/mpas_component.puml

Chris-Hibbert · 2021-09-28T16:58:30Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_component.puml

+
+    ap -d-> pooln
+
+    offer --> conf1: execute method


What is this part of the diagram (offer(), validateTerms(), executeMethod) trying to convey?

Everything else appears to be either the ways a client of the APIs can interact with methods on the AMM pools or how zoe/zcf manage the balances of the pools.

In regards to Zoe, I wanted to show that it referenced the contract and in invoked the contract functionality, however I may have it backwards. What is the best way to represent offer() does stuff with the respective seats here?

Here's a quick sketch. Zoe holds funds in escrow, and has the user seats. each user seat is paired with a ZCF seat in the corresponding ZCF vat.

A ZCF vat runs a single contract (in this case MPAS). MPAS has multiple pools, each of which knows two balances that reflect a claim on escrowed funds in Zoe.

When a seat is created for a transaction (addLiquidity or adjustBalances are the two examples here) it interacts with the conctract code, reallocations are done, and the user gets something back.

The publicFacet, creatorFacet, etc. could also be pictured within the ZCF vat, since they're facets of the contract.

We can jump on a call if you want to wave my hands and point at the parts of the diagram.

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

Chris-Hibbert · 2021-09-28T20:32:30Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

+return ""creatorFacet""
+== Pool Addition SML:BCK==
+autonumber 10
+pa -> mpas: ""addPool()""


In order to get from creating an instance to adding a pool, the creator has to publish the instance of the contract somewhere. Otherwise, even though the addPool() method is "public", no one would be able to find it. It is completely possible in this model to create an AMM, and add pools, and only tell friends about it.

Is it possible to enumerate or directly reference this contract somehow through reflection or other means? Or does it need to be explicitly published?

It needs to be explicitly published. We have a Board on which references to such things can be shared, but it takes an explicit action to make them visible.

It's a crucial part of the security model. We don't want to treat instances of contracts like they're ambient authority, even though we do share them widely for convenience in some circumstances.

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

Chris-Hibbert · 2021-09-28T20:40:51Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

+zoe -> psA: add/remove funds
+zoe -> usA: add/remove funds


I would draw a clearer distinction between what poolSeat.incrementBy() and .decrementBy() are doing in the contract and funds movement. The reallocate() call (line 121) actually moves assets between seats, while incrementBy() and .decrementBy() are moving amounts between staging descriptions.

No, that's not even right. Zoe holds all the funds in a single purse per brand, and only has allocations for each seat. Zoe is careful to keep the accounts balanced as it moves amounts between accounts, and contracts can't add to balances unless the changes satisfy rights conservation, so the contracts can neither create nor destroy assets, they can only move amounts between accounts maintained by Zoe. When a seat exits, the owner can withdraw the amount the associated balance describes.

Bottom line: we shouldn't talk about moving "funds" when we're talking about actions within a contract.

Chris-Hibbert · 2021-09-28T20:42:19Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

+pu -> usA: ""getOfferResult()""
+return result of failure or success


I think these would be better shown as interactions with Zoe. Zoe gets the results from ZCF/the contract and can return them to the user even if the contract fails.

[actually, in the context of my later comments, if the user seat is moved to be visibly part of zoe, this would be fine.]

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

Chris-Hibbert · 2021-09-28T20:57:37Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

+zoe -> usA: add/remove funds
+zoe -> zoe: ""reallocate(seat, poolSeat)""
+
+usA --> pu: ""userSeat"" ref


I would put UserSeat in a combined (vertical) box with Zoe. Each UserSeat is paired with a ZcfSeat; they're two objects with access to the same state. This diagram shows only one of the zcfSeats that interact during a swap. There's a seat (pair) for each pool, and there's a seat (named seat in swap.js) that represents the assets the user's assets. The pool seats are persistent through the life of the contract, the user seats are new for each swap (created by the invitation as you show.)

I think box, endbox lets you draw a box. see "Participants encompass" here .

I moved the userSeat to be closer to Zoe to hopefully capture this.

Chris-Hibbert · 2021-10-01T16:40:42Z

docs/threat_models/gen.sh

@@ -0,0 +1,9 @@
+#!/bin/sh


Oh, useful! I'm going to have to crib from this for the governance diagrams.

Chris-Hibbert · 2021-10-01T16:46:27Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

+return ""creatorFacet""
+== Pool Addition SML:BCK==
+autonumber 10
+pa -> mpas: ""addPool()""


It needs to be explicitly published. We have a Board on which references to such things can be shared, but it takes an explicit action to make them visible.

It's a crucial part of the security model. We don't want to treat instances of contracts like they're ambient authority, even though we do share them widely for convenience in some circumstances.

Chris-Hibbert · 2021-10-01T17:48:45Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

+
+end
+
+group requesting a SwapOut an invitation


Suggested change

group requesting a SwapOut an invitation

group requesting an invitation for a SwapOut

Chris-Hibbert · 2021-10-01T17:52:51Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_sequence.puml

+usA --> zoe: ""userSeat""
+zoe -> pool: ""getPoolSeat()""
+create control "poolSeat" as psA
+pool -> psA: create ""poolSeat""


There's no "Create poolSeat" call at this point in the transaction. I would actually show the poolSeat as having the same lifetime as the Pool. It's created when the pool comes into existence, and holds the pools assets.

Chris-Hibbert · 2021-10-01T18:29:06Z

docs/threat_models/smart_contracts/multipool_autoswap/mpas_component.puml

+
+    ap -d-> pooln
+
+    offer --> conf1: execute method


Here's a quick sketch. Zoe holds funds in escrow, and has the user seats. each user seat is paired with a ZCF seat in the corresponding ZCF vat.

A ZCF vat runs a single contract (in this case MPAS). MPAS has multiple pools, each of which knows two balances that reflect a claim on escrowed funds in Zoe.

When a seat is created for a transaction (addLiquidity or adjustBalances are the two examples here) it interacts with the conctract code, reallocations are done, and the user gets something back.

The publicFacet, creatorFacet, etc. could also be pictured within the ZCF vat, since they're facets of the contract.

We can jump on a call if you want to wave my hands and point at the parts of the diagram.

Following #3888, add a shell script to generate the diagrams. Rename the files for better consistency. Check in new .pngs, since the .pumls were recently updated.

* docs: regenerate governance diagrams Following #3888, add a shell script to generate the diagrams. Rename the files for better consistency. Check in new .pngs, since the .pumls were recently updated. * chore: add a final newline

jessysaurusrex added the documentation Improvements or additions to documentation label Sep 24, 2021

jessysaurusrex requested a review from katelynsills September 24, 2021 19:42

katelynsills approved these changes Sep 27, 2021

View reviewed changes

jessysaurusrex force-pushed the amuletsec/initial_threat_model_diagrams branch 2 times, most recently from dfec2f1 to 5b728aa Compare September 27, 2021 22:04

docs: add initial threat model diagrams for multipool autoswap and tr…

f8c675a

…easury

jessysaurusrex force-pushed the amuletsec/initial_threat_model_diagrams branch from 5b728aa to f8c675a Compare September 27, 2021 22:05

katelynsills requested a review from Chris-Hibbert September 28, 2021 03:19

Chris-Hibbert reviewed Sep 28, 2021

View reviewed changes

docs: update diagrams to encompass review feedback

edf8b14

jessysaurusrex force-pushed the amuletsec/initial_threat_model_diagrams branch from f50100f to edf8b14 Compare October 1, 2021 03:24

Chris-Hibbert reviewed Oct 1, 2021

View reviewed changes

Chris-Hibbert added a commit that referenced this pull request Oct 4, 2021

docs: regenerate governance diagrams

968ef98

Following #3888, add a shell script to generate the diagrams. Rename the files for better consistency. Check in new .pngs, since the .pumls were recently updated.

Chris-Hibbert mentioned this pull request Oct 4, 2021

docs: regenerate governance diagrams #3927

Merged

Chris-Hibbert added a commit that referenced this pull request Oct 4, 2021

docs: regenerate governance diagrams

fbc50cf

Following #3888, add a shell script to generate the diagrams. Rename the files for better consistency. Check in new .pngs, since the .pumls were recently updated.

jessysaurusrex added 2 commits October 7, 2021 14:10

docs: update diagrams to encompass review feedback #2

1c37852

Merge branch 'master' into amuletsec/initial_threat_model_diagrams

2844238

jessysaurusrex merged commit 1ab71f7 into master Oct 7, 2021

jessysaurusrex deleted the amuletsec/initial_threat_model_diagrams branch October 7, 2021 21:37

jessysaurusrex added the threat modeling label Oct 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: add initial threat model diagrams for multipool autoswap and treasury #3888

docs: add initial threat model diagrams for multipool autoswap and treasury #3888

jessysaurusrex commented Sep 24, 2021

katelynsills commented Sep 24, 2021

jessysaurusrex commented Sep 24, 2021

katelynsills left a comment

katelynsills Sep 27, 2021

jessysaurusrex Sep 27, 2021

katelynsills Sep 28, 2021

katelynsills Sep 27, 2021

katelynsills Sep 27, 2021

jessysaurusrex Sep 27, 2021

katelynsills Sep 28, 2021

jessysaurusrex Oct 1, 2021

katelynsills Sep 27, 2021

jessysaurusrex Sep 27, 2021

katelynsills Sep 28, 2021

jessysaurusrex Oct 1, 2021

katelynsills commented Sep 28, 2021

Chris-Hibbert Sep 28, 2021

jessysaurusrex Oct 1, 2021

Chris-Hibbert Oct 1, 2021

Chris-Hibbert Sep 28, 2021

jessysaurusrex Oct 1, 2021

Chris-Hibbert Oct 1, 2021

Chris-Hibbert Sep 28, 2021

Chris-Hibbert Sep 28, 2021

Chris-Hibbert Sep 28, 2021

jessysaurusrex Oct 1, 2021

Chris-Hibbert Oct 1, 2021

Chris-Hibbert Oct 1, 2021

Chris-Hibbert Oct 1, 2021

Chris-Hibbert Oct 1, 2021

Chris-Hibbert Oct 1, 2021

		pu -> usA: ""getOfferResult()""
		return result of failure or success

	group requesting a SwapOut an invitation
	group requesting an invitation for a SwapOut

docs: add initial threat model diagrams for multipool autoswap and treasury #3888

docs: add initial threat model diagrams for multipool autoswap and treasury #3888

Conversation

jessysaurusrex commented Sep 24, 2021

katelynsills commented Sep 24, 2021

jessysaurusrex commented Sep 24, 2021

katelynsills left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katelynsills commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment