Design flaw in Zoe: stateful seats and staged allocations

[katelynsills]

This is an over-arching issue to capture the architectural design flaw introduced in #3184. #3184 made significant usability and readability gains, at least on the surface, but has since resulted in at least 3 bugs of high severity.

Importantly, until this design flaw is corrected, we can expect more bugs involving staged allocations.

Design prior to #3184

The earlier design created seatStagings - objects which represented a particular staged allocation on a seat. zcf.reallocate acted on seatStagings, not seats. This meant that multiple seatStagings could be reallocated independently of each other, even for the same seat. Moreover, seatStagings could be dropped without introducing bugs within Zoe, even though this pattern is not encouraged.

Here's an example of usage in multipoolAutoswap:

const seatStaging = seat.stage(
  harden({
    In: AmountMath.subtract(amountIn, reducedAmountIn),
    Out: amountOut,
  }),
);

const poolBrandInStaging = brandInPool.stageSeat({
  Secondary: AmountMath.add(
    brandInPool.getSecondaryAmount(),
    reducedAmountIn,
  ),
  Central: AmountMath.subtract(
    brandInPool.getCentralAmount(),
    reducedCentralAmount,
  ),
});

const poolBrandOutStaging = brandOutPool.stageSeat({
  Central: AmountMath.add(
    brandOutPool.getCentralAmount(),
    reducedCentralAmount,
  ),
  Secondary: AmountMath.subtract(
    brandOutPool.getSecondaryAmount(),
    amountOut,
  ),
});

zcf.reallocate(poolBrandInStaging, poolBrandOutStaging, seatStaging);  

Design after #3184

What is currently on master for multipoolAutoswap is a lot clearer:

brandOutPoolSeat.decrementBy({ Secondary: amountOut });
seat.incrementBy({ Out: amountOut });

brandInPoolSeat.decrementBy({ Central: reducedCentralAmount });
brandOutPoolSeat.incrementBy({ Central: reducedCentralAmount });

seat.decrementBy({ In: reducedAmountIn });
brandInPoolSeat.incrementBy({ Secondary: reducedAmountIn });

zcf.reallocate(brandOutPoolSeat, brandInPoolSeat, seat);

The code can easily be audited for rights conservation violations, whereas the previous design could not be. For example, see this analysis of multipoolAutoswap where a rights conservation bug was found only after deep inspection.

On the surface, this code seemed a lot more auditable, but under the surface, problems were introduced. Being able to audit for rights conservation violations at the contract level is great, but only helps if bugs aren't introduced in Zoe.

We had at least three bugs introduced in Zoe as a result of this change:

1. Bug: zcfMint.mintGains wrongly errors if any seats have staged allocations #3613 (fixed)
2. AMM trades fail - Error#125: rights were not conserved for brand [object Alleged: RUN brand] #3800 (open)
3. bug(Zoe): rights conservation and offer safety can be violated due to zcfMint interaction with staged allocations #3848 (fixed)

#3613 resulted from an interaction of zcfMint with stagedAllocations. zcfMint.mintGains calls reallocateInternal after incrementing the zcfSeat with the assets to mint. zcf.reallocate also calls reallocateInternal after doing some work. Thinking mostly of the zcf.reallocate usage of reallocateInternal, we had required that all seats with staged allocations should be included in reallocateInternal. This was intended as a safety measure to ensure that contract code does not unintentionally forget about any seats and their staged allocations. This caused a problem for zcfMint.mintGains, where the common usage is in the middle of preparing staged allocations, prior to the reallocation that would use them. mintGains was erroring if there were any prior outstanding staged allocations, but this was incorrect behavior because having outstanding staged allocations is common. The "quick fix" was to move this check into reallocate only, such that the mintGains path never performed the check.

#3800 was the result of an expected, user-caused error in the AMM (MultipoolAutoswap) causing unexpected state. The readability and usability of the new design #3184 depended entirely on the staged allocations being cleared before an offerHandler was called. In other words, the offerHandlers were written as if the seats involved had no previous staged allocations. This expectation was violated when an offer handler errored because the offer was not one that the contract wanted to accept, and this error left stagedAllocations in place for future offers. A potential solution would be to clear all staged allocations on errors, but more thought is required on exactly when this clearing should occur. This issue is still open, requiring more design. The design prior to #3184 would have not had this problem, because prior seatStagings would simply be dropped, and no state regarding staged allocations would continue to stay around after an error.

#3848, the latest bug, is the result of mintGains being able to perform a reallocate for one seat without rights conservation being checked. Before the new design in #3184, this made sense because only the seatStaging that was involved in mintGains would be reallocated against, and minting doesn't conserve rights anyways. Any other seatStagings, even for the same seat, would be unaffected. However, with the new design, the mintGains is adding to a stagedAllocation that may already exist. This means that staged allocations that would violate rights conservation can slip in and become the canonical allocation. Offer safety holds for this particular transaction, but because rights conservation is violated, the offer safety in other offers is violated. Even though "offer safety" might be true in terms of allocations, if rights are not conserved, someone will not be able to get a payout that matches their allocation.

[Tartuffo]

@erights Is this done? The three issues mentioned in the body are all done. And if it is not done, is it really an Epic? If so, please add the dependent issues.

[dckc]

As noted in #6679, the Zoe design flaw is not yet fixed.

[erights]

but has since resulted in at least 3 bugs of high severity.

We just discovered a fourth bug of high severity:
Contract can use stagings to violate payout liveness #7024

[Chris-Hibbert]

#6577 showed how to address this issue, and #6678, #7024, and #7025 did (or planned) the work of propagating it everywhere. This issue is no longer necessary to track things.