Add ExternalCallLib which checks revert data for malicious data

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

@@ -84,6 +84,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

@@ -197,6 +197,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-linear/contracts/aave/AaveLinearPool.sol

@@ -16,6 +16,7 @@ pragma solidity ^0.7.0;
 pragma experimental ABIEncoderV2;
 
 import "@balancer-labs/v2-interfaces/contracts/pool-linear/IStaticAToken.sol";
+import "@balancer-labs/v2-pool-utils/contracts/lib/ExternalCallLib.sol";
 
 import "../LinearPool.sol";
 
@@ -69,10 +70,15 @@ contract AaveLinearPool is LinearPool {
         // except avoiding storing relevant variables in storage for gas reasons.
         // solhint-disable-next-line max-line-length
         // see: https://github.com/aave/protocol-v2/blob/ac58fea62bb8afee23f66197e8bce6d79ecda292/contracts/protocol/tokenization/StaticATokenLM.sol#L255-L257
-        uint256 rate = _lendingPool.getReserveNormalizedIncome(address(getMainToken()));
-
-        // This function returns a 18 decimal fixed point number, but `rate` has 27 decimals (i.e. a 'ray' value)
-        // so we need to convert it.
-        return rate / 10**9;
+        try _lendingPool.getReserveNormalizedIncome(address(getMainToken())) returns (uint256 rate) {
+            // This function returns a 18 decimal fixed point number, but `rate` has 27 decimals (i.e. a 'ray' value)
+            // so we need to convert it.
+            return rate / 10**9;
+        } catch (bytes memory revertData) {
+            // By maliciously reverting here, Aave (or any other contract in the call stack) could trick the Pool into
+            // reporting invalid data to the query mechanism for swaps/joins/exits.
+            // We then check the revert data to ensure this doesn't occur.
+            ExternalCallLib.checkForMaliciousRevert(revertData);
+        }
     }
 }

pkg/pool-linear/contracts/test/MockAaveLendingPool.sol

@@ -0,0 +1,34 @@
+// 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/pool-linear/IStaticAToken.sol";
+
+import "@balancer-labs/v2-pool-utils/contracts/test/MaliciousQueryReverter.sol";
+
+import "@balancer-labs/v2-solidity-utils/contracts/test/TestToken.sol";
+
+contract MockAaveLendingPool is ILendingPool, MaliciousQueryReverter {
+    uint256 private _rate = 1e27;
+
+    function getReserveNormalizedIncome(address) external view override returns (uint256) {
+        if (revertMaliciously) spoofSwapQueryRevert();
+        return _rate;
+    }
+
+    function setReserveNormalizedIncome(uint256 newRate) external {
+        _rate = newRate;
+    }
+}

pkg/pool-linear/contracts/test/MockStaticAToken.sol

@@ -18,17 +18,19 @@ import "@balancer-labs/v2-interfaces/contracts/pool-linear/IStaticAToken.sol";
 
 import "@balancer-labs/v2-solidity-utils/contracts/test/TestToken.sol";
 
-contract MockStaticAToken is TestToken, IStaticAToken, ILendingPool {
-    uint256 private _rate = 1e27;
+contract MockStaticAToken is TestToken, IStaticAToken {
     address private immutable _ASSET;
+    ILendingPool private immutable _lendingPool;
 
     constructor(
         string memory name,
         string memory symbol,
         uint8 decimals,
-        address underlyingAsset
+        address underlyingAsset,
+        ILendingPool lendingPool
     ) TestToken(name, symbol, decimals) {
         _ASSET = underlyingAsset;
+        _lendingPool = lendingPool;
     }
 
     // solhint-disable-next-line func-name-mixedcase
@@ -38,21 +40,13 @@ contract MockStaticAToken is TestToken, IStaticAToken, ILendingPool {
 
     // solhint-disable-next-line func-name-mixedcase
     function LENDING_POOL() external view override returns (ILendingPool) {
-        return ILendingPool(this);
+        return _lendingPool;
     }
 
     function rate() external pure override returns (uint256) {
         revert("Should not call this");
     }
 
-    function getReserveNormalizedIncome(address) external view override returns (uint256) {
-        return _rate;
-    }
-
-    function setReserveNormalizedIncome(uint256 newRate) external {
-        _rate = newRate;
-    }
-
     function deposit(
         address,
         uint256,

pkg/pool-linear/test/AaveLinearPool.test.ts

@@ -29,12 +29,15 @@ describe('AaveLinearPool', function () {
   });
 
   sharedBeforeEach('deploy tokens', async () => {
+    mockLendingPool = await deploy('MockAaveLendingPool');
+
     mainToken = await Token.create('DAI');
-    const wrappedTokenInstance = await deploy('MockStaticAToken', { args: ['cDAI', 'cDAI', 18, mainToken.address] });
+    const wrappedTokenInstance = await deploy('MockStaticAToken', {
+      args: ['cDAI', 'cDAI', 18, mainToken.address, mockLendingPool.address],
+    });
     wrappedToken = await Token.deployedAt(wrappedTokenInstance.address);
 
     tokens = new TokenList([mainToken, wrappedToken]).sort();
-    mockLendingPool = wrappedTokenInstance;
 
     await tokens.mint({ to: [lp, trader], amount: fp(100) });
   });
@@ -82,15 +85,27 @@ describe('AaveLinearPool', function () {
   });
 
   describe('getWrappedTokenRate', () => {
-    it('returns the expected value', async () => {
-      // Reserve's normalised income is stored with 27 decimals (i.e. a 'ray' value)
-      // 1e27 implies a 1:1 exchange rate between main and wrapped token
-      await mockLendingPool.setReserveNormalizedIncome(bn(1e27));
-      expect(await pool.getWrappedTokenRate()).to.be.eq(fp(1));
-
-      // We now double the reserve's normalised income to change the exchange rate to 2:1
-      await mockLendingPool.setReserveNormalizedIncome(bn(2e27));
-      expect(await pool.getWrappedTokenRate()).to.be.eq(fp(2));
+    context('under normal operation', () => {
+      it('returns the expected value', async () => {
+        // Reserve's normalised income is stored with 27 decimals (i.e. a 'ray' value)
+        // 1e27 implies a 1:1 exchange rate between main and wrapped token
+        await mockLendingPool.setReserveNormalizedIncome(bn(1e27));
+        expect(await pool.getWrappedTokenRate()).to.be.eq(fp(1));
+
+        // We now double the reserve's normalised income to change the exchange rate to 2:1
+        await mockLendingPool.setReserveNormalizedIncome(bn(2e27));
+        expect(await pool.getWrappedTokenRate()).to.be.eq(fp(2));
+      });
+    });
+
+    context('when Aave reverts maliciously', () => {
+      sharedBeforeEach('make Aave lending pool start reverting', async () => {
+        await mockLendingPool.setRevertMaliciously(true);
+      });
+
+      it('reverts with MALICIOUS_QUERY_REVERT', async () => {
+        await expect(pool.getWrappedTokenRate()).to.be.revertedWith('MALICIOUS_QUERY_REVERT');
+      });
     });
   });
 

pkg/pool-linear/test/AaveLinearPoolFactory.test.ts

@@ -35,8 +35,12 @@ describe('AaveLinearPoolFactory', function () {
     });
     creationTime = await currentTimestamp();
 
+    const mockLendingPool = await deploy('MockAaveLendingPool');
+
     const mainToken = await Token.create('DAI');
-    const wrappedTokenInstance = await deploy('MockStaticAToken', { args: ['cDAI', 'cDAI', 18, mainToken.address] });
+    const wrappedTokenInstance = await deploy('MockStaticAToken', {
+      args: ['cDAI', 'cDAI', 18, mainToken.address, mockLendingPool.address],
+    });
     const wrappedToken = await Token.deployedAt(wrappedTokenInstance.address);
 
     tokens = new TokenList([mainToken, wrappedToken]).sort();

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

@@ -0,0 +1,55 @@
+// 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;
+
+        // solhint-disable-next-line no-inline-assembly
+        assembly {
+            // 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 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(
+                or(
+                    // BasePool._queryAction
+                    eq(error, 0x43adbafb00000000000000000000000000000000000000000000000000000000),
+                    // Vault.queryBatchSwap
+                    eq(error, 0xfa61cc1200000000000000000000000000000000000000000000000000000000)
+                )
+            ) {
+                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,79 @@
+// 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 {
+    bool public revertMaliciously;
+
+    function setRevertMaliciously(bool enabled) external {
+        revertMaliciously = enabled;
+    }
+
+    function spoofJoinExitQueryRevert() 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 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))
+        }
+    }
+}

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

@@ -0,0 +1,46 @@
+// 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 protectedSwapExternalCall() external payable {
+        try maliciousCallee.spoofSwapQueryRevert()  {} catch (bytes memory revertdata) {
+            ExternalCallLib.checkForMaliciousRevert(revertdata);
+        }
+    }
+
+    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();
+    }
+}