BREAKING CHANGE(ertp): Ensure that ERTP/AmountMath validates inputs

packages/ERTP/docs/INPUT_VALIDATION.md

@@ -0,0 +1,141 @@
+# Input Validation for ERTP - A Walkthrough
+
+## Requirements
+
+We want ERTP to be robust against malicious code in the same vat, so the ERTP must be robust without the kind of checks that we get for free in intervat communication going through `@agoric/marshal` and `@agoric/swingset-vat`. In other words, the only tools at our disposal are being in a Hardened JS environment, and helpers that we import and call directly, such as `passStyleOf` from `@agoric/marshal`. 
+
+## Malicious Behavior
+
+We identify two personas: the creator of an issuerKit, and a user of the asset type. A user is someone who holds a purse or payment of that brand, or describes something in terms of an amount of that brand. A creator might be vulnerable to an attack by users and vice versa. Users might be vulnerable to other users.
+
+The malicious behavior we would like to prevent is:
+
+1. Users stealing funds from other users
+2. Users minting who do not have access to the mint
+3. The creator of an issuerKit revoking assets from the holders of purses and payments
+
+## Entry points in ERTP
+
+There are three entry points in ERTP:
+1. packages/ERTP/src/amountMath.js
+2. packages/ERTP/src/issuerKit.js
+3. packages/ERTP/src/typeGuards.js
+
+This document will only analyze `AmountMath.coerce`, but the
+methodology for validating inputs remains the same for the ERTP functions.
+
+`AmountMath.coerce` takes a `brand` and an `amount` and returns a new `amount`, but throws if the amount does not match the brand, or if the amount is not a valid amount generally.
+
+For `coerce` to work according to spec, if the user passes in an invalid brand or an invalid amount, we must throw. 
+
+## Valid amounts
+
+A valid amount is an object record with two properties: `brand` and
+`value`. `brand` is a Remotable with a few methods, such as
+`isMyIssuer`, but for the purposes of a valid amount, we allow any
+Remotable and do not call any of these methods (we would have to call
+them asynchronously in order to check them, since the `brand` might be
+a remote object, so we do not check them.)
+
+The value is either a bigint Nat in the case of AssetKind.NAT (we formerly allowed numbers for backwards compatibility, but now only bigints are allowed) or an array of Structures in the case of AssetKind.SET. The array of Structures might include any combination of other Structures, “undefined”, “null”, booleans, symbols, strings, pass-by-copy records, pass-by-copy arrays, numbers, bigints,  Remotables, and errors. Importantly, a Structure cannot contain promises. 
+
+## Invalid Amounts
+
+There are a number of ways in which amounts can be invalid:
+
+### Not an object
+
+* **The danger**: we’ll get unexpected failures later on in our use of the amount rather than failing fast at the start as intended.
+
+### Not a CopyRecord
+
+* **The danger**: an object that isn’t a `copyRecord` can have `getters` that return different values at different times, so checking the value is no guarantee that the value will be the same when accessed later. For example:
+
+```js
+const object1 = {};
+let checked = false;
+Object.defineProperty(object1, 'value', {
+ get() {
+   if (checked) {
+     return 1000000n;
+   } else {
+     checked = true;
+     return 1n;
+   }
+ },
+});
+```
+```sh
+> object1.value
+1n
+> object1.value
+1000000n
+```
+
+`Object.freeze(object1)` is not helpful here, as the `getter` is not changing, the value that is being returned is. `harden` also does not prevent `value` from changing. ​​When harden does encounter an accessor property during its traversal, it does not "read" the property, and thereby does not call the getter. Rather, it proceeds to recursively harden the getter and setter themselves. That's because, for an accessor property, its current value isn't, for this purpose, considered part of its API surface, but rather part of the behavior of that API. Thus, we need an additional way to ensure that the value of `checked` cannot change.
+
+`isPassStyleOf` throws on objects with accessor properties like the object defined above.
+
+### Is a CopyRecord and is a proxy
+
+* **The dangers**: 
+  1) A proxy can throw on any property access, but only the first
+     time. Any time it does *not* throw, it *must* return the same
+     value as before. 
+  2) A proxy can mount a reentrancy attack where a proxy handler is running while code is executing, and can reenter the code while the original is still executing. 
+
+Commit points are a good mitigation for #1 - do all of your checks upfront so that any throwing causes you to fail fast, before any state is changed.
+
+Some mitigation for #2 are: 
+1. Create a new object with a spread operator so that the proxy isn't
+   used further. 
+2. Destructure and then never use the original object again. 
+3. Use `pureCopy`. `pureCopy` ensures the object is not a proxy, but `pureCopy` only works for copyRecords that are pure data, meaning no remotables and no promises.
+
+We aim to address proxy-based reentrancy by other, lower-level means, thus making `passStyleOf(amount) === ‘copyRecord’` sufficient protection against proxy-based reentrancy as well, but we should note that proxy-based reentrancy is not a threat across a vat boundary because it requires synchronous access.
+
+## Invalid brands
+
+A brand can be "wrong" if:
+1. It's not a Remotable
+2. Its methods don’t adhere to the expected Brand API
+3. It misbehaves (i.e. answering isMyIssuer with different responses)
+
+In determining whether a brand is "valid", we only check that the brand is a remotable. This means that a brand
+that passes our input validation could still have the wrong API or misbehave.
+
+## Who hardens?
+It is the responsibility of the sender/client (the creator of an
+issuerKit or user of ERTP) to harden. AmountMath does not harden
+inputs.
+
+## The implementation
+
+In `AmountMath.coerce(brand, allegedAmount)`, we do the following:
+1. assert that the `brand` is a remotable
+2. assert that the `allegedAmount` is a `copyRecord`
+3. destructure the `allegedAmount` into `allegedBrand` and
+   `allegedValue`
+4. Assert that the `brand` is identical to the `allegedBrand` 
+5. Call `AmountMath.make(brand, allegedValue)`, which:
+    * Asserts that the `brand` is a remotable, again.
+    * Asserts that the `allegedValue` is a `copyArray` or a `bigint`
+    * Gets the appropriate MathHelpers
+    * Calls `helpers.doCoerce(allegedValue)`, which either asserts
+      that the value is a `Nat bigint` or that the value is a
+      `copyArray` `structure` with no duplicate elements
+11. Return a new `amount`
+
+Thus, we ensure that the `brand` is valid by checking that it is a
+remotable. We ensure that the `allegedAmount` is a copyRecord with
+valid `brand` and `value` properties.
+
+If `allegedAmount` were a proxy, we would either throw in step 3 (the
+destructuring), or we successfully get both the `allegedBrand` and
+`allegedValue` and never touch the proxy again. Currently, we do not
+attempt to prevent proxy-based reentrancy, so this is the full extent
+of our defense.
+
+
+
+