feat: add ExternalCallLib which checks revert data for malicious data

pkg/balancer-js/src/utils/errors.ts

@@ -83,6 +83,7 @@ const balancerErrorCodes: Record<string, string> = {
   '354': 'ADD_OR_REMOVE_BPT',
   '355': 'INVALID_CIRCUIT_BREAKER_BOUNDS',
   '356': 'CIRCUIT_BREAKER_TRIPPED',
+  '357': 'MALICIOUS_QUERY_REVERT',
   '400': 'REENTRANCY',
   '401': 'SENDER_NOT_ALLOWED',
   '402': 'PAUSED',

pkg/interfaces/contracts/solidity-utils/helpers/BalancerErrors.sol

@@ -196,6 +196,7 @@ library Errors {
     uint256 internal constant ADD_OR_REMOVE_BPT = 354;
     uint256 internal constant INVALID_CIRCUIT_BREAKER_BOUNDS = 355;
     uint256 internal constant CIRCUIT_BREAKER_TRIPPED = 356;
+    uint256 internal constant MALICIOUS_QUERY_REVERT = 357;
 
     // Lib
     uint256 internal constant REENTRANCY = 400;

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

@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+pragma solidity ^0.7.0;
+
+import "@balancer-labs/v2-interfaces/contracts/solidity-utils/helpers/BalancerErrors.sol";
+
+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.
+
+            // We only forward the revert reason if it doesn't match the error sigature "QueryError(uint256,uint256[])",
+            // 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)) {
+                revert(add(errorData, 0x20), errorLength)
+            }
+        }
+
+        // We expect the assembly block to revert for all non-malicious errors.
+        _revert(Errors.MALICIOUS_QUERY_REVERT);
+    }
+}

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

@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+pragma solidity ^0.7.0;
+
+contract MaliciousQueryReverter {
+    function spoofQueryRevert() public pure {
+        uint256[] memory tokenAmounts = new uint256[](2);
+        tokenAmounts[0] = 1;
+        tokenAmounts[1] = 2;
+
+        uint256 bptAmount = 420;
+
+        // solhint-disable-next-line no-inline-assembly
+        assembly {
+            // We will return a raw representation of `bptAmount` and `tokenAmounts` in memory, which is composed of
+            // a 32-byte uint256, followed by a 32-byte for the array length, and finally the 32-byte uint256 values
+            // Because revert expects a size in bytes, we multiply the array length (stored at `tokenAmounts`) by 32
+            let size := mul(mload(tokenAmounts), 32)
+
+            // We store the `bptAmount` in the previous slot to the `tokenAmounts` array. We can make sure 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.
+            let start := sub(tokenAmounts, 0x20)
+            mstore(start, bptAmount)
+
+            // We send one extra value for the error signature "QueryError(uint256,uint256[])" which is 0x43adbafb
+            // We use the previous slot to `bptAmount`.
+            mstore(sub(start, 0x20), 0x0000000000000000000000000000000000000000000000000000000043adbafb)
+            start := sub(start, 0x04)
+
+            // When copying from `tokenAmounts` into returndata, we copy the additional 68 bytes to also return
+            // the `bptAmount`, the array's length, and the error signature.
+            revert(start, add(size, 68))
+        }
+    }
+
+    function getRate() external pure returns (uint256) {
+        spoofQueryRevert();
+        return 0;
+    }
+}

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

@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+pragma solidity ^0.7.0;
+
+import "./MaliciousQueryReverter.sol";
+import "../lib/ExternalCallLib.sol";
+
+contract MockExternalCaller {
+    MaliciousQueryReverter maliciousCallee;
+
+    constructor(MaliciousQueryReverter callee) {
+        maliciousCallee = callee;
+    }
+
+    function protectedExternalCall() external payable {
+        try maliciousCallee.spoofQueryRevert()  {} catch (bytes memory revertdata) {
+            ExternalCallLib.checkForMaliciousRevert(revertdata);
+        }
+    }
+
+    function unprotectedExternalCall() external payable {
+        maliciousCallee.spoofQueryRevert();
+    }
+}

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

@@ -0,0 +1,38 @@
+import { Contract } from 'ethers';
+
+import { deploy } from '@balancer-labs/v2-helpers/src/contract';
+import { expect } from 'chai';
+import { solidityKeccak256 } from 'ethers/lib/utils';
+
+describe('ExternalCallLib', function () {
+  let maliciousReverter: Contract;
+  let caller: Contract;
+
+  sharedBeforeEach(async function () {
+    maliciousReverter = await deploy('MaliciousQueryReverter');
+    caller = await deploy('MockExternalCaller', { args: [maliciousReverter.address] });
+  });
+
+  describe('making external call to a malicious contract', () => {
+    context('when call is protected', () => {
+      it('reverts with MALICIOUS_QUERY_REVERT', async () => {
+        await expect(caller.protectedExternalCall()).to.be.revertedWith('MALICIOUS_QUERY_REVERT');
+      });
+    });
+
+    context('when call is unprotected', () => {
+      it('bubbles up original revert data', async () => {
+        const maliciousErrorSignature = solidityKeccak256(['string'], ['QueryError(uint256,uint256[])']).slice(0, 10);
+
+        try {
+          const tx = await caller.unprotectedExternalCall();
+          await tx.wait();
+        } catch (e: any) {
+          const revertData = e.data;
+
+          expect(revertData.slice(0, 10)).to.be.eq(maliciousErrorSignature);
+        }
+      });
+    });
+  });
+});