feat: add protection for swap as well as join/exits

pkg/pool-utils/contracts/lib/ExternalCallLib.sol

@@ -20,18 +20,27 @@ library ExternalCallLib {
     function checkForMaliciousRevert(bytes memory errorData) internal pure {
         uint256 errorLength = errorData.length;
         assembly {
-            // If the first 4 bytes match the error signature "QueryError(uint256,uint256[])" then this
-            // error is attempting to impersonate the mechanism used by `BasePool._queryAction`, injecting bogus data.
-            // This can result in loss of funds if the return value of `BasePool._queryAction` is then used in a later
-            // calculation.
+            // If the first 4 bytes match the selector for one of the error signatures used by `BasePool._queryAction`
+            // or `Vault.queryBatchSwap` then this error is attempting to impersonate the query mechanism used by these
+            // contracts in order to inject bogus data. This can result in loss of funds if the return value is then
+            // used in a later calculation.
+            //
+            // We then want to reject the following error signatures:
+            // - `QueryError(uint256,uint256[])` (used by `BasePool._queryAction`)
+            // - `QueryError(int256[])` (used by `Vault.queryBatchSwap`)
 
-            // We only forward the revert reason if it doesn't match the error sigature "QueryError(uint256,uint256[])",
+            // We only forward the revert reason if it doesn't match the any of the selectors for these error sigatures,
             // otherwise we return a new error message flagging that the revert was malicious.
             let error := and(
                 mload(add(errorData, 0x20)),
                 0xffffffff00000000000000000000000000000000000000000000000000000000
             )
-            if iszero(eq(error, 0x43adbafb00000000000000000000000000000000000000000000000000000000)) {
+            if iszero(
+                or(
+                    eq(error, 0x43adbafb00000000000000000000000000000000000000000000000000000000), // BasePool._queryAction
+                    eq(error, 0xfa61cc1200000000000000000000000000000000000000000000000000000000) // Vault.queryBatchSwap
+                )
+            ) {
                 revert(add(errorData, 0x20), errorLength)
             }
         }

pkg/pool-utils/contracts/test/MaliciousQueryReverter.sol

@@ -15,7 +15,7 @@
 pragma solidity ^0.7.0;
 
 contract MaliciousQueryReverter {
-    function spoofQueryRevert() public pure {
+    function spoofJoinExitQueryRevert() public pure {
         uint256[] memory tokenAmounts = new uint256[](2);
         tokenAmounts[0] = 1;
         tokenAmounts[1] = 2;
@@ -46,8 +46,33 @@ contract MaliciousQueryReverter {
         }
     }
 
+    function spoofSwapQueryRevert() public pure {
+        int256[] memory deltas = new int256[](2);
+        deltas[0] = 1;
+        deltas[1] = 2;
+
+        // solhint-disable-next-line no-inline-assembly
+        assembly {
+            // We will return a raw representation of the array in memory, which is composed of a 32 byte length,
+            // followed by the 32 byte int256 values. Because revert expects a size in bytes, we multiply the array
+            // length (stored at `deltas`) by 32.
+            let size := mul(mload(deltas), 32)
+
+            // We send one extra value for the error signature "QueryError(int256[])" which is 0xfa61cc12.
+            // We store it in the previous slot to the `deltas` array. We know there will be at least one available
+            // slot due to how the memory scratch space works.
+            // We can safely overwrite whatever is stored in this slot as we will revert immediately after that.
+            mstore(sub(deltas, 0x20), 0x00000000000000000000000000000000000000000000000000000000fa61cc12)
+            let start := sub(deltas, 0x04)
+
+            // When copying from `deltas` into returndata, we copy an additional 36 bytes to also return the array's
+            // length and the error signature.
+            revert(start, add(size, 36))
+        }
+    }
+
     function getRate() external pure returns (uint256) {
-        spoofQueryRevert();
+        spoofSwapQueryRevert();
         return 0;
     }
 }

pkg/pool-utils/contracts/test/MockExternalCaller.sol

@@ -24,13 +24,23 @@ contract MockExternalCaller {
         maliciousCallee = callee;
     }
 
-    function protectedExternalCall() external payable {
-        try maliciousCallee.spoofQueryRevert()  {} catch (bytes memory revertdata) {
+    function protectedSwapExternalCall() external payable {
+        try maliciousCallee.spoofSwapQueryRevert()  {} catch (bytes memory revertdata) {
             ExternalCallLib.checkForMaliciousRevert(revertdata);
         }
     }
 
-    function unprotectedExternalCall() external payable {
-        maliciousCallee.spoofQueryRevert();
+    function unprotectedSwapExternalCall() external payable {
+        maliciousCallee.spoofSwapQueryRevert();
+    }
+
+    function protectedJoinExitExternalCall() external payable {
+        try maliciousCallee.spoofJoinExitQueryRevert()  {} catch (bytes memory revertdata) {
+            ExternalCallLib.checkForMaliciousRevert(revertdata);
+        }
+    }
+
+    function unprotectedJoinExitExternalCall() external payable {
+        maliciousCallee.spoofJoinExitQueryRevert();
     }
 }

pkg/pool-utils/test/ExternalCallLib.test.ts

@@ -13,10 +13,33 @@ describe('ExternalCallLib', function () {
     caller = await deploy('MockExternalCaller', { args: [maliciousReverter.address] });
   });
 
-  describe('making external call to a malicious contract', () => {
+  describe('when calling into a malicious contract in a swap', () => {
     context('when call is protected', () => {
       it('reverts with MALICIOUS_QUERY_REVERT', async () => {
-        await expect(caller.protectedExternalCall()).to.be.revertedWith('MALICIOUS_QUERY_REVERT');
+        await expect(caller.protectedSwapExternalCall()).to.be.revertedWith('MALICIOUS_QUERY_REVERT');
+      });
+    });
+
+    context('when call is unprotected', () => {
+      it('bubbles up original revert data', async () => {
+        const maliciousErrorSignature = solidityKeccak256(['string'], ['QueryError(int256[])']).slice(0, 10);
+
+        try {
+          const tx = await caller.unprotectedSwapExternalCall();
+          await tx.wait();
+        } catch (e: any) {
+          const revertData = e.data;
+
+          expect(revertData.slice(0, 10)).to.be.eq(maliciousErrorSignature);
+        }
+      });
+    });
+  });
+
+  describe('when calling into a malicious contract in a join/exit', () => {
+    context('when call is protected', () => {
+      it('reverts with MALICIOUS_QUERY_REVERT', async () => {
+        await expect(caller.protectedJoinExitExternalCall()).to.be.revertedWith('MALICIOUS_QUERY_REVERT');
       });
     });
 
@@ -25,7 +48,7 @@ describe('ExternalCallLib', function () {
         const maliciousErrorSignature = solidityKeccak256(['string'], ['QueryError(uint256,uint256[])']).slice(0, 10);
 
         try {
-          const tx = await caller.unprotectedExternalCall();
+          const tx = await caller.unprotectedJoinExitExternalCall();
           await tx.wait();
         } catch (e: any) {
           const revertData = e.data;