From 258e56cdfc72aade780ac2b12388eedcbffb8ba1 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 11 Nov 2022 17:58:15 +0000 Subject: [PATCH] feat: add ExternalCallLib which checks revert data for malicious data --- pkg/balancer-js/src/utils/errors.ts | 1 + .../solidity-utils/helpers/BalancerErrors.sol | 1 + .../contracts/lib/ExternalCallLib.sol | 42 +++++++++++++++ .../contracts/test/MaliciousQueryReverter.sol | 53 +++++++++++++++++++ .../contracts/test/MockExternalCaller.sol | 36 +++++++++++++ pkg/pool-utils/test/ExternalCallLib.test.ts | 38 +++++++++++++ 6 files changed, 171 insertions(+) create mode 100644 pkg/pool-utils/contracts/lib/ExternalCallLib.sol create mode 100644 pkg/pool-utils/contracts/test/MaliciousQueryReverter.sol create mode 100644 pkg/pool-utils/contracts/test/MockExternalCaller.sol create mode 100644 pkg/pool-utils/test/ExternalCallLib.test.ts diff --git a/pkg/balancer-js/src/utils/errors.ts b/pkg/balancer-js/src/utils/errors.ts index b90b8d3982..6202451d0d 100644 --- a/pkg/balancer-js/src/utils/errors.ts +++ b/pkg/balancer-js/src/utils/errors.ts @@ -83,6 +83,7 @@ const balancerErrorCodes: Record = { '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 index 3471a90e41..8eac914d55 100644 --- a/pkg/interfaces/contracts/solidity-utils/helpers/BalancerErrors.sol +++ b/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; diff --git a/pkg/pool-utils/contracts/lib/ExternalCallLib.sol b/pkg/pool-utils/contracts/lib/ExternalCallLib.sol new file mode 100644 index 0000000000..53dbef09a8 --- /dev/null +++ b/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 . + +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); + } +} diff --git a/pkg/pool-utils/contracts/test/MaliciousQueryReverter.sol b/pkg/pool-utils/contracts/test/MaliciousQueryReverter.sol new file mode 100644 index 0000000000..d79aff5ed2 --- /dev/null +++ b/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 . + +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; + } +} diff --git a/pkg/pool-utils/contracts/test/MockExternalCaller.sol b/pkg/pool-utils/contracts/test/MockExternalCaller.sol new file mode 100644 index 0000000000..20f83d154d --- /dev/null +++ b/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 . + +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(); + } +} diff --git a/pkg/pool-utils/test/ExternalCallLib.test.ts b/pkg/pool-utils/test/ExternalCallLib.test.ts new file mode 100644 index 0000000000..6d1f3788a0 --- /dev/null +++ b/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); + } + }); + }); + }); +});