Add ExternalCallLib which checks revert data for malicious data (#2004)

* feat: add ExternalCallLib which checks revert data for malicious data * feat: add protection for swap as well as join/exits * style: linting * feat: integrate ExternalCallLib into AaveLinearPool * style: linter * Apply suggestions from code review Co-authored-by: Nicolás Venturo <[email protected]> * refactor: rename `bubbleUpNonMaliciousRevert` to `bubbleUpNonMaliciousRevert` * test: remove duplication of test code in ExternalCallLib.test.ts * test: add tests to ensure that protection doesn't mangle non-malicious revert data * style: linter * test: test LinearPool handles malicious joins/exits as well as swaps * style: move constructor test to top of file * test: ensure that asset manager addresses are non-zero * test: add test to prevent rebalancing in case of malicious external call revert Co-authored-by: Nicolás Venturo <[email protected]>
balancer · Nov 15, 2022 · 551df1b · 551df1b
1 parent aa992dd
commit 551df1b
Show file tree

Hide file tree

Showing 11 changed files with 467 additions and 46 deletions.
diff --git a/pkg/balancer-js/src/utils/errors.ts b/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',

diff --git a/pkg/interfaces/contracts/solidity-utils/helpers/BalancerErrors.sol b/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;

diff --git a/pkg/pool-linear/contracts/aave/AaveLinearPool.sol b/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.bubbleUpNonMaliciousRevert(revertData);
+        }
     }
 }
diff --git a/pkg/pool-linear/contracts/test/MockAaveLendingPool.sol b/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) {
+        maybeRevertMaliciously();
+        return _rate;
+    }
+
+    function setReserveNormalizedIncome(uint256 newRate) external {
+        _rate = newRate;
+    }
+}
diff --git a/pkg/pool-linear/contracts/test/MockStaticAToken.sol b/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,

diff --git a/pkg/pool-linear/test/AaveLinearPool.test.ts b/pkg/pool-linear/test/AaveLinearPool.test.ts
@@ -11,9 +11,17 @@ import Token from '@balancer-labs/v2-helpers/src/models/tokens/Token';
 import TokenList from '@balancer-labs/v2-helpers/src/models/tokens/TokenList';
 import LinearPool from '@balancer-labs/v2-helpers/src/models/pools/linear/LinearPool';
 
-import { deploy } from '@balancer-labs/v2-helpers/src/contract';
+import { deploy, deployedAt } from '@balancer-labs/v2-helpers/src/contract';
 import Vault from '@balancer-labs/v2-helpers/src/models/vault/Vault';
-import { ZERO_ADDRESS } from '@balancer-labs/v2-helpers/src/constants';
+import { MAX_UINT256, ZERO_ADDRESS } from '@balancer-labs/v2-helpers/src/constants';
+import { SwapKind } from '@balancer-labs/balancer-js';
+
+enum RevertType {
+  DoNotRevert,
+  NonMalicious,
+  MaliciousSwapQuery,
+  MaliciousJoinExitQuery,
+}
 
 describe('AaveLinearPool', function () {
   let vault: Vault;
@@ -29,12 +37,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) });
   });
@@ -64,13 +75,32 @@ describe('AaveLinearPool', function () {
     pool = await LinearPool.deployedAt(event.args.pool);
   });
 
+  describe('constructor', () => {
+    it('reverts if the mainToken is not the ASSET of the wrappedToken', async () => {
+      const otherToken = await Token.create('USDC');
+
+      await expect(
+        poolFactory.create(
+          'Balancer Pool Token',
+          'BPT',
+          otherToken.address,
+          wrappedToken.address,
+          bn(0),
+          POOL_SWAP_FEE_PERCENTAGE,
+          owner.address
+        )
+      ).to.be.revertedWith('TOKENS_MISMATCH');
+    });
+  });
+
   describe('asset managers', () => {
     it('sets the same asset manager for main and wrapped token', async () => {
       const poolId = await pool.getPoolId();
 
       const { assetManager: firstAssetManager } = await vault.getPoolTokenInfo(poolId, tokens.first);
       const { assetManager: secondAssetManager } = await vault.getPoolTokenInfo(poolId, tokens.second);
 
+      expect(firstAssetManager).to.not.equal(ZERO_ADDRESS);
       expect(firstAssetManager).to.equal(secondAssetManager);
     });
 
@@ -82,33 +112,75 @@ 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));
+      });
     });
-  });
 
-  describe('constructor', () => {
-    it('reverts if the mainToken is not the ASSET of the wrappedToken', async () => {
-      const otherToken = await Token.create('USDC');
+    context('when Aave reverts maliciously to impersonate a swap query', () => {
+      sharedBeforeEach('make Aave lending pool start reverting', async () => {
+        await mockLendingPool.setRevertType(RevertType.MaliciousSwapQuery);
+      });
 
-      await expect(
-        poolFactory.create(
-          'Balancer Pool Token',
-          'BPT',
-          otherToken.address,
-          wrappedToken.address,
-          bn(0),
-          POOL_SWAP_FEE_PERCENTAGE,
-          owner.address
-        )
-      ).to.be.revertedWith('TOKENS_MISMATCH');
+      it('reverts with MALICIOUS_QUERY_REVERT', async () => {
+        await expect(pool.getWrappedTokenRate()).to.be.revertedWith('MALICIOUS_QUERY_REVERT');
+      });
+    });
+
+    context('when Aave reverts maliciously to impersonate a join/exit query', () => {
+      sharedBeforeEach('make Aave lending pool start reverting', async () => {
+        await mockLendingPool.setRevertType(RevertType.MaliciousJoinExitQuery);
+      });
+
+      it('reverts with MALICIOUS_QUERY_REVERT', async () => {
+        await expect(pool.getWrappedTokenRate()).to.be.revertedWith('MALICIOUS_QUERY_REVERT');
+      });
+    });
+  });
+
+  describe('rebalancing', () => {
+    context('when Aave reverts maliciously to impersonate a swap query', () => {
+      let rebalancer: Contract;
+      sharedBeforeEach('provide initial liquidity to pool', async () => {
+        const poolId = await pool.getPoolId();
+
+        await tokens.approve({ to: vault, amount: fp(100), from: lp });
+        await vault.instance.connect(lp).swap(
+          {
+            poolId,
+            kind: SwapKind.GivenIn,
+            assetIn: mainToken.address,
+            assetOut: pool.address,
+            amount: fp(10),
+            userData: '0x',
+          },
+          { sender: lp.address, fromInternalBalance: false, recipient: lp.address, toInternalBalance: false },
+          0,
+          MAX_UINT256
+        );
+      });
+
+      sharedBeforeEach('deploy and initialize pool', async () => {
+        const poolId = await pool.getPoolId();
+        const { assetManager } = await vault.getPoolTokenInfo(poolId, tokens.first);
+        rebalancer = await deployedAt('AaveLinearPoolRebalancer', assetManager);
+      });
+
+      sharedBeforeEach('make Aave lending pool start reverting', async () => {
+        await mockLendingPool.setRevertType(RevertType.MaliciousSwapQuery);
+      });
+
+      it('reverts with MALICIOUS_QUERY_REVERT', async () => {
+        await expect(rebalancer.rebalance(trader.address)).to.be.revertedWith('MALICIOUS_QUERY_REVERT');
+      });
     });
   });
 });
diff --git a/pkg/pool-linear/test/AaveLinearPoolFactory.test.ts b/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();

diff --git a/pkg/pool-utils/contracts/lib/ExternalCallLib.sol b/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 bubbleUpNonMaliciousRevert(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 bubble up the revert reason if it doesn't match the any of the selectors for these error
+            // sigatures, otherwise we revert with 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);
+    }
+}