fix: fix bug which breaks rights conservation and offer safety guarantees

packages/zoe/src/contractFacet/zcfSeat.js

@@ -113,51 +113,28 @@ export const createSeatManager = (
    * and so can be used internally for reallocations that violate
    * conservation.
    *
-   * @type {ReallocateInternal}
+   * @type {ReallocateForZCFMint}
    */
-  const reallocateInternal = seats => {
-    // This check was already done in `reallocate`, but
-    // zcfMint.mintGains and zcfMint.burnLosses call
-    // `reallocateInternal` directly without calling `reallocate`, so
-    // we must check here as well.
-    seats.forEach(assertActive);
-    seats.forEach(assertStagedAllocation);
-
-    // Keep track of seats used so far in this call, to prevent aliasing.
-    const zcfSeatsSoFar = new Set();
-
-    seats.forEach(seat => {
-      assert(
-        zcfSeatToSeatHandle.has(seat),
-        X`The seat ${seat} was not recognized`,
-      );
-      assert(
-        !zcfSeatsSoFar.has(seat),
-        X`Seat (${seat}) was already an argument to reallocate`,
-      );
-      zcfSeatsSoFar.add(seat);
-    });
-
+  const reallocateForZCFMint = (zcfSeat, newAllocation) => {
     try {
-      // No side effects above. All conditions checked which could have
-      // caused us to reject this reallocation.
       // COMMIT POINT
       // All the effects below must succeed "atomically". Scare quotes because
       // the eventual send at the bottom is part of this "atomicity" even
       // though its effects happen later. The send occurs in the order of
       // updates from zcf to zoe, its effects must occur immediately in zoe
       // on reception, and must not fail.
       //
-      // Commit the staged allocations (currentAllocation is replaced
-      // for each of the seats) and inform Zoe of the
+      // Commit the newAllocation and inform Zoe of the
       // newAllocation.
 
-      seats.forEach(commitStagedAllocation);
+      activeZCFSeats.set(zcfSeat, newAllocation);
 
-      const seatHandleAllocations = seats.map(seat => {
-        const seatHandle = zcfSeatToSeatHandle.get(seat);
-        return { seatHandle, allocation: seat.getCurrentAllocation() };
-      });
+      const seatHandleAllocations = [
+        {
+          seatHandle: zcfSeatToSeatHandle.get(zcfSeat),
+          allocation: newAllocation,
+        },
+      ];
 
       E(zoeInstanceAdmin).replaceAllocations(seatHandleAllocations);
     } catch (err) {
@@ -208,8 +185,47 @@ export const createSeatManager = (
       X`At least one seat has a staged allocation but was not included in the call to reallocate`,
     );
 
-    // Note COMMIT POINT within reallocateInternal
-    reallocateInternal(seats);
+    // Keep track of seats used so far in this call, to prevent aliasing.
+    const zcfSeatsSoFar = new Set();
+
+    seats.forEach(seat => {
+      assert(
+        zcfSeatToSeatHandle.has(seat),
+        X`The seat ${seat} was not recognized`,
+      );
+      assert(
+        !zcfSeatsSoFar.has(seat),
+        X`Seat (${seat}) was already an argument to reallocate`,
+      );
+      zcfSeatsSoFar.add(seat);
+    });
+
+    try {
+      // No side effects above. All conditions checked which could have
+      // caused us to reject this reallocation.
+      // COMMIT POINT
+      // All the effects below must succeed "atomically". Scare quotes because
+      // the eventual send at the bottom is part of this "atomicity" even
+      // though its effects happen later. The send occurs in the order of
+      // updates from zcf to zoe, its effects must occur immediately in zoe
+      // on reception, and must not fail.
+      //
+      // Commit the staged allocations (currentAllocation is replaced
+      // for each of the seats) and inform Zoe of the
+      // newAllocation.
+
+      seats.forEach(commitStagedAllocation);
+
+      const seatHandleAllocations = seats.map(seat => {
+        const seatHandle = zcfSeatToSeatHandle.get(seat);
+        return { seatHandle, allocation: seat.getCurrentAllocation() };
+      });
+
+      E(zoeInstanceAdmin).replaceAllocations(seatHandleAllocations);
+    } catch (err) {
+      shutdownWithFailure(err);
+      throw err;
+    }
   };
 
   /** @type {MakeZCFSeat} */
@@ -323,7 +339,7 @@ export const createSeatManager = (
   return harden({
     makeZCFSeat,
     reallocate,
-    reallocateInternal,
+    reallocateForZCFMint,
     dropAllReferences,
   });
 };

packages/zoe/src/contractFacet/zcfZygote.js

@@ -17,6 +17,7 @@ import { createSeatManager } from './zcfSeat.js';
 import { makeInstanceRecordStorage } from '../instanceRecordStorage.js';
 import { handlePWarning, handlePKitWarning } from '../handleWarning.js';
 import { makeOfferHandlerStorage } from './offerHandlerStorage.js';
+import { addToAllocation, subtractFromAllocation } from './allocationMath.js';
 
 import '../../exported.js';
 import '../internal-types.js';
@@ -56,7 +57,7 @@ export const makeZCFZygote = (
   const {
     makeZCFSeat,
     reallocate,
-    reallocateInternal,
+    reallocateForZCFMint,
     dropAllReferences,
   } = createSeatManager(
     zoeInstanceAdmin,
@@ -152,31 +153,67 @@ export const makeZCFZygote = (
           zcfSeat = makeEmptySeatKit().zcfSeat;
         }
         const totalToMint = Object.values(gains).reduce(add, empty);
-        zcfSeat.incrementBy(gains);
-        // verifies offer safety
-        assert(zcfSeat.isOfferSafe(zcfSeat.getStagedAllocation()));
-        // No effects above. The following two steps
-        // *should* be committed atomically, but it is not a
-        // disaster if they are not. If we minted only, no one would
-        // ever get those invisibly-minted assets.
+        assert(
+          !zcfSeat.hasExited(),
+          `zcfSeat must be active to mint gains for the zcfSeat`,
+        );
+        const allocationPlusGains = addToAllocation(
+          zcfSeat.getCurrentAllocation(),
+          gains,
+        );
+
+        // Increment the stagedAllocation if it exists so that the
+        // stagedAllocation is kept up to the currentAllocation
+        if (zcfSeat.hasStagedAllocation()) {
+          zcfSeat.incrementBy(gains);
+        }
+
+        // Offer safety should never be able to be violated here, as
+        // we are adding assets. However, we keep this check so that
+        // all reallocations are covered by offer safety checks, and
+        // that any bug within Zoe that may affect this is caught.
+        assert(
+          zcfSeat.isOfferSafe(allocationPlusGains),
+          `The allocation after minting gains ${allocationPlusGains} for the zcfSeat was not offer safe`,
+        );
+        // No effects above, apart from incrementBy. Note COMMIT POINT within
+        // reallocateForZCFMint. The following two steps *should* be
+        // committed atomically, but it is not a disaster if they are
+        // not. If we minted only, no one would ever get those
+        // invisibly-minted assets.
         E(zoeMintP).mintAndEscrow(totalToMint);
-        // Note COMMIT POINT within reallocateInternal.
-        reallocateInternal([zcfSeat]);
+        reallocateForZCFMint(zcfSeat, allocationPlusGains);
         return zcfSeat;
       },
       burnLosses: (losses, zcfSeat) => {
         assertAmountKeywordRecord(losses, 'losses');
         const totalToBurn = Object.values(losses).reduce(add, empty);
-        zcfSeat.decrementBy(losses);
+        assert(
+          !zcfSeat.hasExited(),
+          `zcfSeat must be active to burn losses from the zcfSeat`,
+        );
+        const allocationMinusLosses = subtractFromAllocation(
+          zcfSeat.getCurrentAllocation(),
+          losses,
+        );
+
+        // Decrement the stagedAllocation if it exists so that the
+        // stagedAllocation is kept up to the currentAllocation
+        if (zcfSeat.hasStagedAllocation()) {
+          zcfSeat.decrementBy(losses);
+        }
+
         // verifies offer safety
-        assert(zcfSeat.isOfferSafe(zcfSeat.getStagedAllocation()));
-        // No effects above.
-        // Note COMMIT POINT within reallocateInternal.
-        // The following two steps
-        // *should* be committed atomically, but it is not a disaster
-        // if they are not. If we only commit the stagedAllocation no
-        // one would ever get the unburned assets.
-        reallocateInternal([zcfSeat]);
+        assert(
+          zcfSeat.isOfferSafe(allocationMinusLosses),
+          `The allocation after burning losses ${allocationMinusLosses}for the zcfSeat was not offer safe`,
+        );
+        // No effects above, apart from decrementBy. Note COMMIT POINT within
+        // reallocateForZCFMint. The following two steps *should* be
+        // committed atomically, but it is not a disaster if they are
+        // not. If we only commit the allocationMinusLosses no one would
+        // ever get the unburned assets.
+        reallocateForZCFMint(zcfSeat, allocationMinusLosses);
         E(zoeMintP).withdrawAndBurn(totalToBurn);
       },
     });

packages/zoe/src/internal-types.js

@@ -278,8 +278,9 @@
  */
 
 /**
- * @callback ReallocateInternal
- * @param {ZCFSeat[]} seats
+ * @callback ReallocateForZCFMint
+ * @param {ZCFSeat} zcfSeat
+ * @param {Allocation} newAllocation
  * @returns {void}
  */
 
@@ -295,7 +296,7 @@
  * @param {ShutdownWithFailure} shutdownWithFailure
  * @returns {{ makeZCFSeat: MakeZCFSeat,
     reallocate: Reallocate,
-    reallocateInternal: ReallocateInternal,
+    reallocateForZCFMint: ReallocateForZCFMint,
     dropAllReferences: DropAllReferences }}
  */