VAL-138 Inconsistent global withdraw state after cancellation (#178)

* Add failing test demonstrating issue

* Add initial solution; test passes

* Refactor implementation. Update prior tests to reflect new signature of PoolLib function.

* Remove hardhat console

contracts/controllers/WithdrawController.sol

@@ -325,18 +325,29 @@ contract WithdrawController is IWithdrawController, BeaconImplementation {
         uint256 currentPeriod = withdrawPeriod();
 
         // Update the requested amount from the user
-        _withdrawState[owner] = PoolLib.calculateWithdrawStateForCancellation(
-            _currentWithdrawState(owner),
-            currentPeriod,
+        IPoolWithdrawState memory lenderState = _currentWithdrawState(owner);
+        uint256 requestedSharesPrior = lenderState.requestedShares;
+        uint256 eligibleSharesPrior = lenderState.eligibleShares;
+
+        lenderState = PoolLib.calculateWithdrawStateForCancellation(
+            lenderState,
             shares
         );
+        _withdrawState[owner] = lenderState;
 
         // Update the global amount
-        _globalWithdrawState = PoolLib.calculateWithdrawStateForCancellation(
+        // We adjust the requested and eligible balances based on the
+        // lenders adjusted state post-cancellation.
+        _globalWithdrawState = PoolLib.progressWithdrawState(
             _globalWithdrawState,
-            currentPeriod,
-            shares
+            currentPeriod
         );
+        _globalWithdrawState.requestedShares -=
+            requestedSharesPrior -
+            lenderState.requestedShares;
+        _globalWithdrawState.eligibleShares -=
+            eligibleSharesPrior -
+            lenderState.eligibleShares;
     }
 
     /*//////////////////////////////////////////////////////////////

contracts/libraries/PoolLib.sol

@@ -474,11 +474,9 @@ library PoolLib {
      */
     function calculateWithdrawStateForCancellation(
         IPoolWithdrawState memory state,
-        uint256 currentPeriod,
         uint256 cancelledShares
     ) public pure returns (IPoolWithdrawState memory updatedState) {
-        updatedState = progressWithdrawState(state, currentPeriod);
-
+        updatedState = state;
         // Decrease the requested, eligible shares count, and ensure the "latestRequestPeriod"
         // is set to the current request period.
         if (updatedState.requestedShares > cancelledShares) {

contracts/mocks/PoolLibTestWrapper.sol

@@ -234,13 +234,11 @@ contract PoolLibTestWrapper is ERC20("PoolLibTest", "PLT") {
 
     function calculateWithdrawStateForCancellation(
         IPoolWithdrawState memory state,
-        uint256 currentPeriod,
         uint256 cancelledShares
     ) public pure returns (IPoolWithdrawState memory) {
         return
             PoolLib.calculateWithdrawStateForCancellation(
                 state,
-                currentPeriod,
                 cancelledShares
             );
     }

test/libraries/PoolLib.test.ts

@@ -723,7 +723,6 @@ describe("PoolLib", () => {
       expect(
         await poolLibWrapper.calculateWithdrawStateForCancellation(
           withdrawState,
-          0,
           22
         )
       ).to.deep.equal(
@@ -745,11 +744,7 @@ describe("PoolLib", () => {
       });
 
       await expect(
-        poolLibWrapper.calculateWithdrawStateForCancellation(
-          withdrawState,
-          1,
-          33
-        )
+        poolLibWrapper.calculateWithdrawStateForCancellation(withdrawState, 33)
       ).to.be.revertedWith("Pool: Invalid cancelled shares");
     });
   });

test/scenarios/pool/withdraw-request.test.ts

@@ -28,6 +28,39 @@ describe("Withdraw Requests", () => {
 
     return {
       pool,
+      poolController,
+      liquidityAsset,
+      withdrawController,
+      poolAdmin,
+      aliceLender,
+      bobLender
+    };
+  }
+
+  async function loadPoolFixtureNoFees() {
+    const [poolAdmin, aliceLender, bobLender] = await ethers.getSigners();
+    const { pool, liquidityAsset, poolController, withdrawController } =
+      await deployPool({
+        poolAdmin: poolAdmin,
+        settings: {
+          requestCancellationFeeBps: 0,
+          requestFeeBps: 0,
+          withdrawGateBps: 0
+        }
+      });
+
+    // activate the pool
+    await activatePool(pool, poolAdmin, liquidityAsset);
+
+    // deposit 100 tokens from Alice
+    await depositToPool(pool, aliceLender, liquidityAsset, 100);
+
+    // deposit 100 tokens from Bob
+    await depositToPool(pool, bobLender, liquidityAsset, 100);
+
+    return {
+      pool,
+      poolController,
       liquidityAsset,
       withdrawController,
       poolAdmin,
@@ -187,4 +220,60 @@ describe("Withdraw Requests", () => {
       await withdrawController.requestedBalanceOf(aliceLender.address)
     ).to.equal(0);
   });
+
+  it("cancellations affect the global withdraw state consistently with the individuals", async () => {
+    const { pool, aliceLender, bobLender, withdrawController } =
+      await loadFixture(loadPoolFixtureNoFees);
+    const { withdrawRequestPeriodDuration } = await pool.settings();
+
+    expect(await pool.maxRedeemRequest(aliceLender.address)).to.equal(100);
+    expect(await pool.maxRedeemRequest(bobLender.address)).to.equal(100);
+
+    // Request half
+    await pool.connect(aliceLender).requestRedeem(50);
+    await pool.connect(bobLender).requestRedeem(50);
+
+    // Advance to next period
+    await time.increase(withdrawRequestPeriodDuration);
+
+    // Request other half
+    await pool.connect(aliceLender).requestRedeem(50);
+    await pool.connect(bobLender).requestRedeem(50);
+
+    // We now expect each lender to have 50 requested and 50 eligible
+    expect(
+      await withdrawController.eligibleBalanceOf(aliceLender.address)
+    ).to.equal(50);
+    expect(
+      await withdrawController.requestedBalanceOf(aliceLender.address)
+    ).to.equal(50);
+
+    expect(
+      await withdrawController.eligibleBalanceOf(bobLender.address)
+    ).to.equal(50);
+    expect(
+      await withdrawController.requestedBalanceOf(bobLender.address)
+    ).to.equal(50);
+
+    // We expect the global requested balance and the global eligible balance to be 100 and 100
+    expect(await withdrawController.totalRequestedBalance()).to.equal(100);
+    expect(await withdrawController.totalEligibleBalance()).to.equal(100);
+
+    // Alice now cancels the 100 shares (her full balance)
+    await pool.connect(aliceLender).cancelRedeemRequest(100);
+
+    // We expect her requested balance to be 0 and her eligible balance to be 0
+    expect(
+      await withdrawController.requestedBalanceOf(aliceLender.address)
+    ).to.equal(0);
+    expect(
+      await withdrawController.eligibleBalanceOf(aliceLender.address)
+    ).to.equal(0);
+
+    // Since Alice had 50 eligible and 50 requested, we should expect those same amounts
+    // to be decremented from the global withdraw state.
+    // So we expect the global state to now have 50 requested and 50 eligible
+    expect(await withdrawController.totalRequestedBalance()).to.equal(50);
+    expect(await withdrawController.totalEligibleBalance()).to.equal(50);
+  });
 });