fix: fix Zoe bug in which offer safety can be violated (#1115)

* test: demonstrate an attack by a zoe contract to be fixed in facets The bug was introduced in "filter proposal give and want by sparseKeywords" #1076. It allows a grifter contract (demonstrated here) to slip assets away from a victim one keyword at a time. The victim is left with neither their give or their want. The solution in facets is to reinstate the insistance that offer safety be checked against all keywords. We provide the feature that #1076 was aiming for by allowing reallocations that don't impact keywords in an offer's proposal. * fix: Fix the Zoe offer-safety bug demonstrated by the grifter attack The fix is to insist that all offers retain offer safety across all keywords. The desired ability to rearrange values outside of the offer's keywords can be satisfied by explicitly allowing rearrangements that don't impact any of the proposal's keywords. This commit includes the bug fix but not a test that demonstrates the vulnerability. * test: correct the test to pass now that the bug is fixed. * fix: run offer safety check over the entire new allocation, not just the sparse keywords version. Co-authored-by: Kate Sills <[email protected]>
Agoric · May 15, 2020 · 39d6ae2 · 39d6ae2
1 parent b8d462e
commit 39d6ae2
Show file tree

Hide file tree

Showing 6 changed files with 164 additions and 133 deletions.
diff --git a/packages/zoe/src/offerSafety.js b/packages/zoe/src/offerSafety.js
@@ -24,15 +24,11 @@ function isOfferSafeForOffer(
   proposal,
   newAmountKeywordRecord,
 ) {
-  const isGTEByKeyword = ([keyword, amount]) => {
-    if (!Object.keys(amountMathKeywordRecord).includes(keyword)) {
-      return false;
-    }
-    return amountMathKeywordRecord[keyword].isGTE(
+  const isGTEByKeyword = ([keyword, amount]) =>
+    amountMathKeywordRecord[keyword].isGTE(
       newAmountKeywordRecord[keyword],
       amount,
     );
-  };
 
   // For this allocation to count as a full refund, the allocated
   // amount must be greater than or equal to what was originally
@@ -46,24 +42,4 @@ function isOfferSafeForOffer(
   return refundOk || winningsOk;
 }
 
-/**
- * @param  {object} amountMathKeywordRecord - a record with keywords
- * as keys and amountMath as values
- * @param  {proposal[]} proposals - an array of records which are the
- * proposal for a single player. Each proposal has keys `give`,
- * `want`, and `exit`.
- * @param  {newAmountKeywordRecord[]} newAllocations - an array of
- * records. Each of the records (amountKeywordRecord) has keywords for
- * keys and the values are the amount that a single user will get.
- */
-const isOfferSafeForAll = (
-  amountMathKeywordRecord,
-  proposals,
-  newAllocations,
-) =>
-  proposals.every((proposal, i) =>
-    isOfferSafeForOffer(amountMathKeywordRecord, proposal, newAllocations[i]),
-  );
-
-// `isOfferSafeForOffer` is only exported for testing
-export { isOfferSafeForOffer, isOfferSafeForAll };
+export { isOfferSafeForOffer };
diff --git a/packages/zoe/src/state.js b/packages/zoe/src/state.js
@@ -76,19 +76,12 @@ const makeOfferTable = () => {
       },
       updateAmounts: (offerHandles, newAllocations) =>
         offerHandles.map((offerHandle, i) => {
-          // newAmountKeywordRecords may be based on sparse keywords,
-          // so we don't want to replace the allocation entirely
+          // newAllocation can replace the old allocation entirely
           const newAllocation = newAllocations[i];
-          const { updater, currentAllocation: pastAllocation } = table.get(
-            offerHandle,
-          );
-          const currentAllocation = {
-            ...pastAllocation,
-            ...newAllocation,
-          };
-          updater.updateState(currentAllocation);
+          const { updater } = table.get(offerHandle);
+          updater.updateState(newAllocation);
           return table.update(offerHandle, {
-            currentAllocation,
+            currentAllocation: newAllocation,
           });
         }),
     });

diff --git a/packages/zoe/src/zoe.js b/packages/zoe/src/zoe.js
@@ -18,11 +18,10 @@ import {
   objToArray,
   objToArrayAssertFilled,
   filterObj,
-  filterObjOkIfMissing,
   filterFillAmounts,
   assertSubset,
 } from './objArrayConversion';
-import { isOfferSafeForAll } from './offerSafety';
+import { isOfferSafeForOffer } from './offerSafety';
 import { areRightsConserved } from './rightsConservation';
 import { evalContractCode } from './evalContractCode';
 import { makeTables } from './state';
@@ -449,48 +448,32 @@ const makeZoe = (additionalEndowments = {}) => {
           offerHandles.length >= 2,
           details`reallocating must be done over two or more offers`,
         );
+
+        // Set sparseKeywords if undefined.
         const { issuerKeywordRecord } = instanceTable.get(instanceHandle);
         const allKeywords = getKeywords(issuerKeywordRecord);
         if (sparseKeywords === undefined) {
           sparseKeywords = allKeywords;
         }
 
-        const newAmountMatrix = newAllocations.map(amountObj =>
-          objToArrayAssertFilled(amountObj, sparseKeywords),
-        );
-
-        const offerRecords = offerTable.getOffers(offerHandles);
-
+        // 1) ensure that rights are conserved overall
         const amountMathKeywordRecord = contractFacet.getAmountMaths(
           sparseKeywords,
         );
-
-        // Filter proposal `want` and `give` by sparseKeywords
-        const filterProposalBySparseKeywords = proposal => {
-          return harden({
-            give: filterObjOkIfMissing(proposal.give, sparseKeywords),
-            want: filterObjOkIfMissing(proposal.want, sparseKeywords),
-          });
-        };
-
-        const proposals = offerRecords.map(offerRecord =>
-          filterProposalBySparseKeywords(offerRecord.proposal),
+        const amountMathsArray = objToArray(
+          amountMathKeywordRecord,
+          sparseKeywords,
         );
-
-        const currentAmountMatrix = offerRecords.map(({ handle }) => {
+        const currentAmountMatrix = offerHandles.map(handle => {
           const filteredAmounts = contractFacet.getCurrentAllocation(
             handle,
             sparseKeywords,
           );
           return objToArray(filteredAmounts, sparseKeywords);
         });
-
-        const amountMathsArray = objToArray(
-          amountMathKeywordRecord,
-          sparseKeywords,
+        const newAmountMatrix = newAllocations.map(amountObj =>
+          objToArrayAssertFilled(amountObj, sparseKeywords),
         );
-
-        // 1) ensure that rights are conserved
         assert(
           areRightsConserved(
             amountMathsArray,
@@ -500,14 +483,42 @@ const makeZoe = (additionalEndowments = {}) => {
           details`Rights are not conserved in the proposed reallocation`,
         );
 
-        // 2) ensure 'offer safety' for each player
-        assert(
-          isOfferSafeForAll(amountMathKeywordRecord, proposals, newAllocations),
-          details`The proposed reallocation was not offer safe`,
+        // 2) Ensure 'offer safety' for each offer separately.
+
+        // Make the potential reallocation and test for offer safety
+        // by comparing the potential reallocation to the proposal.
+        const makePotentialReallocation = (
+          offerHandle,
+          sparseKeywordsAllocation,
+        ) => {
+          const { proposal, currentAllocation } = offerTable.get(offerHandle);
+          const potentialReallocation = harden({
+            ...currentAllocation,
+            ...sparseKeywordsAllocation,
+          });
+          const proposalKeywords = [
+            ...getKeywords(proposal.want),
+            ...getKeywords(proposal.give),
+          ];
+          assert(
+            isOfferSafeForOffer(
+              contractFacet.getAmountMaths(proposalKeywords),
+              proposal,
+              potentialReallocation,
+            ),
+            details`The proposed reallocation was not offer safe`,
+          );
+
+          // The reallocation passes the offer safety check
+          return potentialReallocation;
+        };
+
+        const reallocations = offerHandles.map((offerHandle, i) =>
+          makePotentialReallocation(offerHandle, newAllocations[i]),
         );
 
         // 3) save the reallocation
-        offerTable.updateAmounts(offerHandles, harden(newAllocations));
+        offerTable.updateAmounts(offerHandles, reallocations);
       },
 
       complete: offerHandles => {

diff --git a/packages/zoe/test/unitTests/contracts/grifter.js b/packages/zoe/test/unitTests/contracts/grifter.js
@@ -0,0 +1,52 @@
+// @ts-check
+
+import harden from '@agoric/harden';
+
+// Eventually will be importable from '@agoric/zoe-contract-support'
+import { makeZoeHelpers } from '../../../src/contractSupport';
+
+/** @typedef {import('../zoe').ContractFacet} ContractFacet */
+
+// zcf is the Zoe Contract Facet, i.e. the contract-facing API of Zoe
+export const makeContract = harden(
+  /** @param {ContractFacet} zcf */ zcf => {
+    const { assertKeywords, checkHook } = makeZoeHelpers(zcf);
+    assertKeywords(harden(['Asset', 'Price']));
+
+    const makeAccompliceInvite = firstOfferHandle => {
+      const {
+        proposal: { want: wantProposal },
+      } = zcf.getOffer(firstOfferHandle);
+
+      return zcf.makeInvitation(
+        offerHandle => {
+          const firstHandleAlloc = zcf.getCurrentAllocation(firstOfferHandle);
+          // safe because it doesn't change give, so they can still get a refund
+          const vicProposal = { Price: firstHandleAlloc.Price };
+          const stepOne = [wantProposal, vicProposal];
+          // safe because it doesn't change want, so winningsOK looks true
+          const offerHandles = [firstOfferHandle, offerHandle];
+          zcf.reallocate(offerHandles, stepOne, ['Price']);
+          zcf.complete(harden(offerHandles));
+        },
+        'tantalizing offer',
+        harden({
+          customProperties: {
+            Price: wantProposal.Price,
+          },
+        }),
+      );
+    };
+
+    const firstOfferExpected = harden({
+      want: { Price: null },
+    });
+
+    return harden({
+      invite: zcf.makeInvitation(
+        checkHook(makeAccompliceInvite, firstOfferExpected),
+        'firstOffer',
+      ),
+    });
+  },
+);
diff --git a/packages/zoe/test/unitTests/contracts/test-grifter.js b/packages/zoe/test/unitTests/contracts/test-grifter.js
@@ -0,0 +1,61 @@
+// eslint-disable-next-line import/no-extraneous-dependencies
+import { test } from 'tape-promise/tape';
+// eslint-disable-next-line import/no-extraneous-dependencies
+import bundleSource from '@agoric/bundle-source';
+
+import harden from '@agoric/harden';
+
+import { makeZoe } from '../../..';
+// TODO: Remove setupBasicMints and rename setupBasicMints2
+import { setup } from '../setupBasicMints';
+
+const grifterRoot = `${__dirname}/grifter`;
+
+test('zoe - grifter tries to steal; prevented by offer safety', async t => {
+  t.plan(1);
+  // Setup zoe and mints
+  const { moola, moolaR, moolaMint, bucksR, bucks } = setup();
+  const zoe = makeZoe({ require });
+  // Pack the contract.
+  const { source, moduleFormat } = await bundleSource(grifterRoot);
+  const installationHandle = zoe.install(source, moduleFormat);
+
+  const issuerKeywordRecord = harden({
+    Asset: bucksR.issuer,
+    Price: moolaR.issuer,
+  });
+
+  const malloryInvite = zoe.makeInstance(
+    installationHandle,
+    issuerKeywordRecord,
+  );
+
+  // Mallory doesn't need any money
+  const malloryProposal = harden({
+    want: { Price: moola(37) },
+  });
+  const { outcome: vicInviteP } = await zoe.offer(
+    malloryInvite,
+    malloryProposal,
+    harden({}),
+  );
+
+  const vicMoolaPayment = moolaMint.mintPayment(moola(37));
+  const vicProposal = harden({
+    give: { Price: moola(37) },
+    want: { Asset: bucks(24) },
+    exit: { onDemand: null },
+  });
+  const vicPayments = { Price: vicMoolaPayment };
+  const { outcome: vicOutcomeP } = await zoe.offer(
+    vicInviteP,
+    vicProposal,
+    vicPayments,
+  );
+
+  t.rejects(
+    vicOutcomeP,
+    /The proposed reallocation was not offer safe/,
+    `vicOffer is rejected`,
+  );
+});
diff --git a/packages/zoe/test/unitTests/test-offerSafety.js b/packages/zoe/test/unitTests/test-offerSafety.js
@@ -2,7 +2,7 @@
 import { test } from 'tape-promise/tape';
 import harden from '@agoric/harden';
 
-import { isOfferSafeForOffer, isOfferSafeForAll } from '../../src/offerSafety';
+import { isOfferSafeForOffer } from '../../src/offerSafety';
 import { setup } from './setupBasicMints';
 
 // Potential outcomes:
@@ -237,65 +237,3 @@ test('isOfferSafeForOffer - empty proposal', t => {
     t.assert(false, e);
   }
 });
-
-// All users get exactly what they wanted
-test('isOfferSafeForAll - All users get what they wanted', t => {
-  t.plan(1);
-  try {
-    const { moolaR, simoleanR, bucksR, moola, simoleans, bucks } = setup();
-    const amountMathKeywordRecord = harden({
-      A: moolaR.amountMath,
-      B: simoleanR.amountMath,
-      C: bucksR.amountMath,
-    });
-    const proposal = harden({
-      want: { B: simoleans(6) },
-      give: { A: moola(1), C: bucks(7) },
-    });
-    const proposals = [proposal, proposal, proposal];
-    const amounts = harden({ A: moola(0), B: simoleans(6), C: bucks(0) });
-    const amountKeywordRecords = [amounts, amounts, amounts];
-    t.ok(
-      isOfferSafeForAll(
-        amountMathKeywordRecord,
-        proposals,
-        amountKeywordRecords,
-      ),
-    );
-  } catch (e) {
-    t.assert(false, e);
-  }
-});
-
-test(`isOfferSafeForAll - One user doesn't get what they wanted`, t => {
-  t.plan(1);
-  try {
-    const { moolaR, simoleanR, bucksR, moola, simoleans, bucks } = setup();
-    const amountMathKeywordRecord = harden({
-      A: moolaR.amountMath,
-      B: simoleanR.amountMath,
-      C: bucksR.amountMath,
-    });
-    const proposal = harden({
-      want: { B: simoleans(6) },
-      give: { A: moola(1), C: bucks(7) },
-    });
-    const proposals = [proposal, proposal, proposal];
-    const amounts = harden({ A: moola(0), B: simoleans(6), C: bucks(0) });
-    const unsatisfiedAmounts = harden({
-      A: moola(0),
-      B: simoleans(4),
-      C: bucks(0),
-    });
-    const amountKeywordRecords = [amounts, amounts, unsatisfiedAmounts];
-    t.notOk(
-      isOfferSafeForAll(
-        amountMathKeywordRecord,
-        proposals,
-        amountKeywordRecords,
-      ),
-    );
-  } catch (e) {
-    t.assert(false, e);
-  }
-});