add a test that uncovers a withdrawal error

contracts/contracts/strategies/crosschain/CrossChainMasterStrategy.sol

@@ -32,8 +32,6 @@ contract CrossChainMasterStrategy is
         Deposit,
         Withdrawal
     }
-    /// @notice Mapping of nonce to transfer type
-    mapping(uint64 => TransferType) public transferTypeByNonce;
 
     event RemoteStrategyBalanceUpdated(uint256 balance);
     event WithdrawRequested(address indexed asset, uint256 amount);
@@ -175,14 +173,6 @@ contract CrossChainMasterStrategy is
 
         // Should be expecting an acknowledgement
         require(!isNonceProcessed(_nonce), "Nonce already processed");
-        // Only a withdrawal can send tokens to Master strategy
-        require(
-            transferTypeByNonce[_nonce] == TransferType.Withdrawal,
-            "Expecting withdrawal"
-        );
-
-        // Confirm receipt of tokens from Withdraw command
-        _markNonceAsProcessed(_nonce);
 
         // Now relay to the regular flow
         // NOTE: Calling _onMessageReceived would mean that we are bypassing a
@@ -223,7 +213,6 @@ contract CrossChainMasterStrategy is
         // Get the next nonce
         // Note: reverts if a transfer is pending
         uint64 nonce = _getNextNonce();
-        transferTypeByNonce[nonce] = TransferType.Deposit;
 
         // Set pending amount
         pendingAmount = depositAmount;
@@ -267,7 +256,6 @@ contract CrossChainMasterStrategy is
         // Get the next nonce
         // Note: reverts if a transfer is pending
         uint64 nonce = _getNextNonce();
-        transferTypeByNonce[nonce] = TransferType.Withdrawal;
 
         // Build and send withdrawal message with payload
         bytes memory message = CrossChainStrategyHelper.encodeWithdrawMessage(
@@ -293,8 +281,10 @@ contract CrossChainMasterStrategy is
         virtual
     {
         // Decode the message
-        (uint64 nonce, uint256 balance) = message.decodeBalanceCheckMessage();
-
+        // When transferConfirmation is true, it means that the message is a result of a deposit or a withdrawal
+        // process.
+        (uint64 nonce, uint256 balance, bool transferConfirmation) = message
+            .decodeBalanceCheckMessage();
         // Get the last cached nonce
         uint64 _lastCachedNonce = lastTransferNonce;
 
@@ -304,32 +294,28 @@ contract CrossChainMasterStrategy is
             return;
         }
 
-        // Check if the nonce has been processed
-        bool processedTransfer = isNonceProcessed(nonce);
-        if (
-            !processedTransfer &&
-            transferTypeByNonce[nonce] == TransferType.Withdrawal
-        ) {
-            // Pending withdrawal is taken care of by _onTokenReceived
-            // Do not update balance due to race conditions
-            return;
+        // A received message nonce not yet processed indicates there is a
+        // deposit or withdrawal in progress.
+        bool transferInProgress = !isNonceProcessed(nonce);
+
+        if (transferInProgress) {
+            if (transferConfirmation) {
+                // Apply the effects of the deposit / withdrawal completion
+                _markNonceAsProcessed(nonce);
+                pendingAmount = 0;
+            } else {
+                // A balanceCheck arrived that is not part of the deposit / withdrawal process
+                // that has been generated on the Remote contract after the deposit / withdrawal which is
+                // still pending. This can happen when the CCTP bridge delivers the messages out of order.
+                // Ignore it, since the pending deposit / withdrawal must first be cofirmed.
+                return;
+            }
         }
 
-        // Update the remote strategy balance always
+        // At this point update the strategy balance the balanceCheck message is either:
+        // - a confirmation of a deposit / withdrawal
+        // - a message that updates balances when no deposit / withdrawal is in progress
         remoteStrategyBalance = balance;
         emit RemoteStrategyBalanceUpdated(balance);
-
-        /**
-         * A deposit is being confirmed.
-         * A withdrawal will always be confirmed if it reaches this point of code.
-         */
-        if (!processedTransfer) {
-            _markNonceAsProcessed(nonce);
-
-            // Effect of confirming a deposit, reset pending amount
-            delete pendingAmount;
-
-            // NOTE: Withdrawal is taken care of by _onTokenReceived
-        }
     }
 }

contracts/contracts/strategies/crosschain/CrossChainRemoteStrategy.sol

@@ -189,7 +189,7 @@ contract CrossChainRemoteStrategy is
         // Send balance check message to the peer strategy
         uint256 balanceAfter = checkBalance(baseToken);
         bytes memory message = CrossChainStrategyHelper
-            .encodeBalanceCheckMessage(lastTransferNonce, balanceAfter);
+            .encodeBalanceCheckMessage(lastTransferNonce, balanceAfter, true);
         _sendMessage(message);
     }
 
@@ -261,7 +261,8 @@ contract CrossChainRemoteStrategy is
             bytes memory message = CrossChainStrategyHelper
                 .encodeBalanceCheckMessage(
                     lastTransferNonce,
-                    balanceAfter - withdrawAmount
+                    balanceAfter - withdrawAmount,
+                    true
                 );
             _sendTokens(withdrawAmount, message);
         } else {
@@ -270,7 +271,11 @@ contract CrossChainRemoteStrategy is
             // - doesn't have sufficient funds to satisfy the withdrawal request
             // In both cases send the balance update message to the peer strategy.
             bytes memory message = CrossChainStrategyHelper
-                .encodeBalanceCheckMessage(lastTransferNonce, balanceAfter);
+                .encodeBalanceCheckMessage(
+                    lastTransferNonce,
+                    balanceAfter,
+                    true
+                );
             _sendMessage(message);
             emit WithdrawFailed(withdrawAmount, usdcBalance);
         }
@@ -349,7 +354,7 @@ contract CrossChainRemoteStrategy is
     {
         uint256 balance = checkBalance(baseToken);
         bytes memory message = CrossChainStrategyHelper
-            .encodeBalanceCheckMessage(lastTransferNonce, balance);
+            .encodeBalanceCheckMessage(lastTransferNonce, balance, false);
         _sendMessage(message);
     }
 

contracts/contracts/strategies/crosschain/CrossChainStrategyHelper.sol

@@ -174,38 +174,44 @@ library CrossChainStrategyHelper {
      *      The message version and type are always encoded in the message.
      * @param nonce The nonce of the balance check
      * @param balance The balance to check
+     * @param transferConfirmation Indicates if the message is a transfer confirmation. This is true
+     *                            when the message is a result of a deposit or a withdrawal.
      * @return The encoded balance check message
      */
-    function encodeBalanceCheckMessage(uint64 nonce, uint256 balance)
-        internal
-        pure
-        returns (bytes memory)
-    {
+    function encodeBalanceCheckMessage(
+        uint64 nonce,
+        uint256 balance,
+        bool transferConfirmation
+    ) internal pure returns (bytes memory) {
         return
             abi.encodePacked(
                 ORIGIN_MESSAGE_VERSION,
                 BALANCE_CHECK_MESSAGE,
-                abi.encode(nonce, balance)
+                abi.encode(nonce, balance, transferConfirmation)
             );
     }
 
     /**
      * @dev Decode the balance check message.
      *      The message version and type are verified in the message.
      * @param message The message to decode
-     * @return The nonce and the balance to check
+     * @return The nonce, the balance and indicates if the message is a transfer confirmation
      */
     function decodeBalanceCheckMessage(bytes memory message)
         internal
         pure
-        returns (uint64, uint256)
+        returns (
+            uint64,
+            uint256,
+            bool
+        )
     {
         verifyMessageVersionAndType(message, BALANCE_CHECK_MESSAGE);
 
-        (uint64 nonce, uint256 balance) = abi.decode(
+        (uint64 nonce, uint256 balance, bool transferConfirmation) = abi.decode(
             getMessagePayload(message),
-            (uint64, uint256)
+            (uint64, uint256, bool)
         );
-        return (nonce, balance);
+        return (nonce, balance, transferConfirmation);
     }
 }

contracts/test/strategies/crosschain/cross-chain-strategy.js

@@ -5,6 +5,7 @@ const {
   crossChainFixtureUnit,
 } = require("../../_fixture");
 const { units } = require("../../helpers");
+const { impersonateAndFund } = require("../../../utils/signers");
 
 const loadFixture = createFixtureLoader(crossChainFixtureUnit);
 
@@ -75,6 +76,10 @@ describe("ForkTest: CrossChainRemoteStrategy", function () {
     await crossChainRemoteStrategy.connect(governor).withdrawAll();
   };
 
+  const sendBalanceUpdateToMaster = async () => {
+    await crossChainRemoteStrategy.connect(governor).sendBalanceUpdate();
+  };
+
   // Checks the diff in the total expected value in the vault
   // (plus accompanying strategy value)
   const assertVaultTotalValue = async (amountExpected) => {
@@ -177,6 +182,7 @@ describe("ForkTest: CrossChainRemoteStrategy", function () {
     await expect(
       await crossChainRemoteStrategy.checkBalance(usdc.address)
     ).to.eq(remoteBalanceAfter);
+
     // Simulate off chain component processing checkBalance message
     await expect(messageTransmitter.processFront())
       .to.emit(crossChainMasterStrategy, "RemoteStrategyBalanceUpdated")
@@ -323,9 +329,126 @@ describe("ForkTest: CrossChainRemoteStrategy", function () {
     ).to.eq(await units("0", usdc));
   });
 
-  it("Should be able to process withdrawal & checkBalance on Remote strategy and in reverse order on master strategy", async function () {});
-
   it("Should fail when a withdrawal too large is requested on the remote strategy", async function () {
-    // TODO: trick master into thinking there is more on remote strategy than is actually there
+    const { messageTransmitter } = fixture;
+    const remoteStrategySigner = await impersonateAndFund(
+      crossChainRemoteStrategy.address
+    );
+
+    await mintToMasterDepositToRemote("1000");
+    await assertVaultTotalValue("1000");
+
+    await directWithdrawFromRemoteStrategy("10");
+
+    // Trick the remote strategy into thinking it has 10 USDC more than it actually does
+    await usdc
+      .connect(remoteStrategySigner)
+      .transfer(vault.address, await units("10", usdc));
+    // Vault has 10 USDC more & Master strategy still thinks it has 1000 USDC
+    await assertVaultTotalValue("1010");
+
+    // This step should fail because the remote strategy no longer holds 1000 USDC
+    await withdrawFromRemoteStrategy("1000");
+
+    // Process on remote strategy
+    await expect(messageTransmitter.processFront())
+      .to.emit(crossChainRemoteStrategy, "WithdrawFailed")
+      .withArgs(await units("1000", usdc), await units("0", usdc));
+
+    // Process on master strategy
+    // This event doesn't get triggerred as the master strategy considers the balance check update
+    // as a race condition, and is exoecting an "on TokenReceived " to be called instead
+
+    // which also causes the master strategy not to update the balance of the remote strategy
+    await expect(messageTransmitter.processFront())
+      .to.emit(crossChainMasterStrategy, "RemoteStrategyBalanceUpdated")
+      .withArgs(await units("990", usdc));
+
+    await expect(await messageTransmitter.messagesInQueue()).to.eq(0);
+
+    await expect(
+      await crossChainRemoteStrategy.checkBalance(usdc.address)
+    ).to.eq(await units("990", usdc));
+
+    await expect(
+      await crossChainMasterStrategy.checkBalance(usdc.address)
+    ).to.eq(await units("990", usdc));
+  });
+
+  it("Should be able to process withdrawal & checkBalance on Remote strategy and in reverse order on master strategy", async function () {
+    const { messageTransmitter } = fixture;
+
+    await mintToMasterDepositToRemote("1000");
+
+    await withdrawFromRemoteStrategy("300");
+
+    // Process on remote strategy
+    await expect(messageTransmitter.processFront());
+    // This sends a second balanceUpdate message to the CCTP bridge
+    await sendBalanceUpdateToMaster();
+
+    await expect(await messageTransmitter.messagesInQueue()).to.eq(2);
+
+    // first process the standalone balanceCheck message - meaning we process messages out of order
+    // this message should be ignored on Master
+    await expect(messageTransmitter.processBack()).to.not.emit(
+      crossChainMasterStrategy,
+      "RemoteStrategyBalanceUpdated"
+    );
+
+    // Second balance update message is part of the deposit / withdrawal process and should be processed
+    await expect(messageTransmitter.processFront())
+      .to.emit(crossChainMasterStrategy, "RemoteStrategyBalanceUpdated")
+      .withArgs(await units("700", usdc));
+
+    await expect(await messageTransmitter.messagesInQueue()).to.eq(0);
+
+    await expect(
+      await crossChainRemoteStrategy.checkBalance(usdc.address)
+    ).to.eq(await units("700", usdc));
+
+    await expect(
+      await crossChainMasterStrategy.checkBalance(usdc.address)
+    ).to.eq(await units("700", usdc));
+
+    await assertVaultTotalValue("1000");
+  });
+
+  it("Should fail on deposit if a previous one has not completed", async function () {
+    await mint("100");
+    await depositToMasterStrategy("50");
+
+    await expect(depositToMasterStrategy("50")).to.be.revertedWith(
+      "Unexpected pending amount"
+    );
+  });
+
+  it("Should fail to withdraw if a previous deposit has not completed", async function () {
+    await mintToMasterDepositToRemote("40");
+    await mint("50");
+    await depositToMasterStrategy("50");
+
+    await expect(withdrawFromRemoteStrategy("40")).to.be.revertedWith(
+      "Pending deposit or withdrawal"
+    );
+  });
+
+  it("Should fail on deposit if a previous withdrawal has not completed", async function () {
+    await mintToMasterDepositToRemote("230");
+    await withdrawFromRemoteStrategy("50");
+
+    await mint("30");
+    await expect(depositToMasterStrategy("30")).to.be.revertedWith(
+      "Pending deposit or withdrawal"
+    );
+  });
+
+  it("Should fail to withdraw if a previous withdrawal has not completed", async function () {
+    await mintToMasterDepositToRemote("230");
+    await withdrawFromRemoteStrategy("50");
+
+    await expect(withdrawFromRemoteStrategy("40")).to.be.revertedWith(
+      "Pending deposit or withdrawal"
+    );
   });
 });