fix(fast-usdc): recover from bad lp proposal #10787

samsiegart · 2024-12-31T00:28:38Z

closes https://github.com/Agoric/agoric-private/issues/234

Description

See issue for context. It also seems related to #10684 because the code path that was triggering the bad state was this:

agoric-sdk/packages/fast-usdc/src/exos/liquidity-pool.js

Lines 272 to 285 in ca25dd5

    
           try { 
        
             this.state.shareWorth = post.shareWorth; 
        
             zcf.atomicRearrange( 
        
               harden([ 
        
                 // zoe guarantees lp has proposal.give allocated 
        
                 [lp, poolSeat, proposal.give], 
        
                 // mintGains() above establishes that mint has post.payouts 
        
                 [mint, lp, post.payouts], 
        
               ]), 
        
             ); 
        
           } catch (cause) { 
        
             // UNTIL #10684: ability to terminate an incarnation w/o terminating the contract 
        
             throw new Error('🚨 cannot commit deposit', { cause }); 
        
           } finally {

As you can see, we were updating the pool state, but then the atomicRearrange failed, so the pool state was left invalid. This PR makes it so that the bad proposal shape is caught by the type guard earlier, so this code path never happens.

The withdraw path was already being handled correctly because the typeguard was specific enough.

If the proposal shape is correct, but the amounts are incorrect, the contract already handles that fine by failing before the state update.

Security Considerations

The bug would allow anyone to send an offer that breaks the liquidity pool.

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

Added a bootstrap test that fails accordingly without the fix.

Upgrade Considerations

None

cloudflare-workers-and-pages · 2024-12-31T00:31:54Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`21d19ba`
Status:	✅ Deploy successful!
Preview URL:	https://406d5d01.agoric-sdk.pages.dev
Branch Preview URL:	https://srs-bad-lp.agoric-sdk.pages.dev

View logs

0xpatrickdev

LGTM. I'll hold approval to give @dckc an opportunity to review as well.

I wonder if we should consider updating the deposit and withdraw handlers:

diff --git a/packages/fast-usdc/src/exos/liquidity-pool.js b/packages/fast-usdc/src/exos/liquidity-pool.js
index 5df4c86bbf..791a68a9ca 100644
--- a/packages/fast-usdc/src/exos/liquidity-pool.js
+++ b/packages/fast-usdc/src/exos/liquidity-pool.js
@@ -257,15 +257,19 @@ export const prepareLiquidityPoolKit = (zone, zcf, USDC, tools) => {
       depositHandler: {
         /** @param {ZCFSeat} lp */
         async handle(lp) {
-          const { shareWorth, shareMint, poolSeat, encumberedBalance } =
-            this.state;
+          const {
+            shareWorth: currShareWorth,
+            shareMint,
+            poolSeat,
+            encumberedBalance,
+          } = this.state;
           const { external } = this.facets;
 
           /** @type {USDCProposalShapes['deposit']} */
           // @ts-expect-error ensured by proposalShape
           const proposal = lp.getProposal();
-          checkPoolBalance(poolSeat, shareWorth, USDC, encumberedBalance);
-          const post = depositCalc(shareWorth, proposal);
+          checkPoolBalance(poolSeat, currShareWorth, USDC, encumberedBalance);
+          const post = depositCalc(currShareWorth, proposal);
 
           // COMMIT POINT
           const mint = shareMint.mintGains(post.payouts);
@@ -280,6 +284,7 @@ export const prepareLiquidityPoolKit = (zone, zcf, USDC, tools) => {
               ]),
             );
           } catch (cause) {
+            this.state.shareWorth = currShareWorth;
             // UNTIL #10684: ability to terminate an incarnation w/o terminating the contract
             throw new Error('🚨 cannot commit deposit', { cause });
           } finally {

0xpatrickdev · 2024-12-31T16:01:19Z

packages/fast-usdc/src/type-guards.js

+    {
+      want: M.splitRecord(
+        {},
+        { PoolShare: makeNatAmountShape(PoolShares) },
+        {},
+      ),
+    },


Good find. I wonder if this should just be:

{ want: { PoolShare: makeNatAmountShape(PoolShares) } },

But I suppose someone could deposit without wanting shares in return? Edit: if that's the case, would wanting 0 pool shares work?

I'm struggling to understand the change. It adds a 3rd arg to splitRecord(...)? What does that do? (looking it up...)

It adds a 3rd arg to splitRecord(...)? What does that do?

I think each arg in M.splitRecord({}, {PoolShares: ... }, {}); says:

can be an empty object

if PoolShares is present, the value must be a USDC amount shape

no other keys allowed

OK... I see rest in splitRecord docs.

I wonder how many other places I have missed this.

yes, 0 pool shares would work.

I was trying to avoid burdening callers with getting access to the PoolShares brand when they are OK relying on the contract to figure it out... but that's really not a normal scenario. In normal usage, clients should not rely on the contract to behave nicely. That's sort of the whole point of offer safety.

Let's be sure to keep the type and the typeguard in sync. If we make the { want: { PoolShare: ... }} part required, we can probably simplify some code that consumes it.

If we make the { want: { PoolShare: ... }} part required, we can probably simplify some code that consumes it.

Done

dckc · 2024-12-31T16:13:36Z

packages/boot/test/fast-usdc/fast-usdc.test.ts

+  const fastLP = agoricNamesRemotes.vbankAsset.FastLP.brand as Brand<'nat'>;
+
+  // Send a bad proposal first to make sure it's recoverable.
+  // (https://github.com/Agoric/agoric-private/issues/234)


Suggested change

// (https://github.com/Agoric/agoric-private/issues/234)

let's avoid pointers from public stuff to private stuff

dckc · 2024-12-31T16:18:02Z

packages/boot/test/fast-usdc/fast-usdc.test.ts

+  const fastLP = agoricNamesRemotes.vbankAsset.FastLP.brand as Brand<'nat'>;
+
+  // Send a bad proposal first to make sure it's recoverable.
+  // (https://github.com/Agoric/agoric-private/issues/234)


Suggested change

// (https://github.com/Agoric/agoric-private/issues/234)

samsiegart

I wonder if we should consider updating the deposit and withdraw handlers

I considered this but I figured we were doing all the validation between checkPoolBalance, depositCalc, and type guards, so I didn't want to mess with the intended logic of this try-catch. Maybe it doesn't hurt to restore the state though? WDYT @dckc ?

samsiegart · 2024-12-31T21:07:43Z

packages/boot/test/fast-usdc/fast-usdc.test.ts

+  const fastLP = agoricNamesRemotes.vbankAsset.FastLP.brand as Brand<'nat'>;
+
+  // Send a bad proposal first to make sure it's recoverable.
+  // (https://github.com/Agoric/agoric-private/issues/234)


samsiegart · 2024-12-31T21:07:53Z

packages/boot/test/fast-usdc/fast-usdc.test.ts

+  const fastLP = agoricNamesRemotes.vbankAsset.FastLP.brand as Brand<'nat'>;
+
+  // Send a bad proposal first to make sure it's recoverable.
+  // (https://github.com/Agoric/agoric-private/issues/234)


samsiegart · 2024-12-31T21:08:08Z

packages/fast-usdc/src/type-guards.js

+    {
+      want: M.splitRecord(
+        {},
+        { PoolShare: makeNatAmountShape(PoolShares) },
+        {},
+      ),
+    },


If we make the { want: { PoolShare: ... }} part required, we can probably simplify some code that consumes it.

Done

dckc

Good catch

mergify · 2025-01-02T01:15:36Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ integration-test-result
☑️ linear-history

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

samsiegart · 2025-01-02T02:15:15Z

@Mergifyio requeue

mergify · 2025-01-02T02:15:18Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

samsiegart requested a review from a team as a code owner December 31, 2024 00:28

samsiegart requested review from AgoricTriage and 0xpatrickdev December 31, 2024 00:28

0xpatrickdev reviewed Dec 31, 2024

View reviewed changes

dckc reviewed Dec 31, 2024

View reviewed changes

samsiegart commented Dec 31, 2024

View reviewed changes

samsiegart requested a review from dckc December 31, 2024 21:16

dckc approved these changes Jan 1, 2025

View reviewed changes

samsiegart added the automerge:rebase Automatically rebase updates, then merge label Jan 2, 2025

samsiegart force-pushed the srs-bad-lp branch from ecd9d40 to 831a76d Compare January 2, 2025 00:23

samsiegart force-pushed the srs-bad-lp branch from 831a76d to c5d30f6 Compare January 2, 2025 02:14

fix(fast-usdc): recover from bad lp proposal

21d19ba

samsiegart force-pushed the srs-bad-lp branch from c5d30f6 to 21d19ba Compare January 2, 2025 02:33

mergify bot merged commit 7750fde into master Jan 2, 2025
81 checks passed

mergify bot deleted the srs-bad-lp branch January 2, 2025 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(fast-usdc): recover from bad lp proposal #10787

fix(fast-usdc): recover from bad lp proposal #10787

samsiegart commented Dec 31, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 31, 2024 •

edited

Loading

0xpatrickdev left a comment

0xpatrickdev Dec 31, 2024 •

edited

Loading

dckc Dec 31, 2024

0xpatrickdev Dec 31, 2024 •

edited

Loading

dckc Dec 31, 2024

dckc Dec 31, 2024

samsiegart Dec 31, 2024

dckc Dec 31, 2024

samsiegart Dec 31, 2024

dckc Dec 31, 2024

samsiegart Dec 31, 2024

samsiegart left a comment

samsiegart Dec 31, 2024

samsiegart Dec 31, 2024

samsiegart Dec 31, 2024

dckc left a comment

mergify bot commented Jan 2, 2025

samsiegart commented Jan 2, 2025

mergify bot commented Jan 2, 2025

	try {
	this.state.shareWorth = post.shareWorth;
	zcf.atomicRearrange(
	harden([
	// zoe guarantees lp has proposal.give allocated
	[lp, poolSeat, proposal.give],
	// mintGains() above establishes that mint has post.payouts
	[mint, lp, post.payouts],
	]),
	);
	} catch (cause) {
	// UNTIL #10684: ability to terminate an incarnation w/o terminating the contract
	throw new Error('🚨 cannot commit deposit', { cause });
	} finally {

fix(fast-usdc): recover from bad lp proposal #10787

fix(fast-usdc): recover from bad lp proposal #10787

Conversation

samsiegart commented Dec 31, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Dec 31, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

0xpatrickdev left a comment

Choose a reason for hiding this comment

0xpatrickdev Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xpatrickdev Dec 31, 2024 • edited Loading

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

samsiegart 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

dckc left a comment

Choose a reason for hiding this comment

mergify bot commented Jan 2, 2025

samsiegart commented Jan 2, 2025

mergify bot commented Jan 2, 2025

❌ This pull request head commit has not been previously disembarked from queue.

samsiegart commented Dec 31, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 31, 2024 •

edited

Loading

0xpatrickdev Dec 31, 2024 •

edited

Loading

0xpatrickdev Dec 31, 2024 •

edited

Loading