From 11b4e67683e1b3807d804e7b7b5bda9981ff80c4 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Tue, 14 May 2024 00:29:25 +0200 Subject: [PATCH] add reentrancy tests --- .../mocks/AttackerWhitelistingContract.sol | 25 +++ .../mocks/BadOperatorWhitelistingContract.sol | 33 ++++ contracts/test/mocks/BeneficiaryContract.sol | 29 +++ .../test/mocks/FakeWhitelistingContract.sol | 67 +++++++ test/validators/whitelist-register.ts | 174 +++++++++++++++++- 5 files changed, 327 insertions(+), 1 deletion(-) create mode 100644 contracts/test/mocks/AttackerWhitelistingContract.sol create mode 100644 contracts/test/mocks/BadOperatorWhitelistingContract.sol create mode 100644 contracts/test/mocks/BeneficiaryContract.sol create mode 100644 contracts/test/mocks/FakeWhitelistingContract.sol diff --git a/contracts/test/mocks/AttackerWhitelistingContract.sol b/contracts/test/mocks/AttackerWhitelistingContract.sol new file mode 100644 index 00000000..a60a2a0b --- /dev/null +++ b/contracts/test/mocks/AttackerWhitelistingContract.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +pragma solidity 0.8.24; + +import "../../interfaces/external/ISSVWhitelistingContract.sol"; +import "../../interfaces/ISSVClusters.sol"; +import "./BeneficiaryContract.sol"; +import "@openzeppelin/contracts/utils/introspection/ERC165.sol"; + +contract AttackerContract { + address private ssvContract; + + constructor(address _ssvContract) { + ssvContract = _ssvContract; + } + + function startAttack( + bytes calldata _publicKey, + uint64[] memory _operatorIds, + bytes calldata _sharesData, + uint256 _amount, + ISSVNetworkCore.Cluster memory _cluserData + ) external { + ISSVClusters(ssvContract).registerValidator(_publicKey, _operatorIds, _sharesData, _amount, _cluserData); + } +} diff --git a/contracts/test/mocks/BadOperatorWhitelistingContract.sol b/contracts/test/mocks/BadOperatorWhitelistingContract.sol new file mode 100644 index 00000000..a018a497 --- /dev/null +++ b/contracts/test/mocks/BadOperatorWhitelistingContract.sol @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +pragma solidity 0.8.24; + +import "../../interfaces/external/ISSVWhitelistingContract.sol"; +import "../../interfaces/ISSVClusters.sol"; +import "./BeneficiaryContract.sol"; +import "@openzeppelin/contracts/utils/introspection/ERC165.sol"; +import "hardhat/console.sol"; + +/// @notice Whitelisted contract that passes the validatity check of supporting ISSVWhitelistingContract +/// and tries to re-enter SSVNetwork.registerValidator function. +contract BadOperatorWhitelistingContract is ERC165 { + BeneficiaryContract private beneficiaryContract; + + constructor(BeneficiaryContract _beneficiaryContract) { + beneficiaryContract = _beneficiaryContract; + } + + function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { + return interfaceId == type(ISSVWhitelistingContract).interfaceId || super.supportsInterface(interfaceId); + } + + fallback() external { + bytes4 selector = bytes4(msg.data); + // only proceed if the function being called is isWhitelisted + if (selector == ISSVWhitelistingContract.isWhitelisted.selector) { + // decode the operator Id + (uint256 operatorId) = abi.decode(msg.data[36:], (uint256)); + // call BeneficiaryContract to withdraw operator earnings + beneficiaryContract.withdrawOperatorEarnings(10000000); + } + } +} diff --git a/contracts/test/mocks/BeneficiaryContract.sol b/contracts/test/mocks/BeneficiaryContract.sol new file mode 100644 index 00000000..bb28086a --- /dev/null +++ b/contracts/test/mocks/BeneficiaryContract.sol @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +pragma solidity 0.8.24; + +import "../../interfaces/external/ISSVWhitelistingContract.sol"; +import "../../interfaces/ISSVOperators.sol"; +import "@openzeppelin/contracts/utils/introspection/ERC165.sol"; +import "hardhat/console.sol"; + +contract BeneficiaryContract { + ISSVOperators private ssvContract; + uint64 private targetOperatorId; + + constructor(ISSVOperators _ssvContract) { + ssvContract = _ssvContract; + } + + function setTargetOperatorId(uint64 _operatorId) external { + targetOperatorId = _operatorId; + } + + function withdrawOperatorEarnings(uint256 amount) external { + // Call SSVNetwork contract, acting as the owner of the operator to try withdraw earnings + ISSVOperators(ssvContract).withdrawOperatorEarnings(targetOperatorId, amount); + } + + function registerOperator() external returns (uint64 operatorId) { + return ISSVOperators(ssvContract).registerOperator("0xcafecafe", 100000000); + } +} diff --git a/contracts/test/mocks/FakeWhitelistingContract.sol b/contracts/test/mocks/FakeWhitelistingContract.sol new file mode 100644 index 00000000..9db27c3d --- /dev/null +++ b/contracts/test/mocks/FakeWhitelistingContract.sol @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +pragma solidity 0.8.24; + +import "../../interfaces/external/ISSVWhitelistingContract.sol"; +import "@openzeppelin/contracts/utils/introspection/ERC165.sol"; + +/// @notice Whitelisted contract that passes the validatity check of supporting ISSVWhitelistingContract +/// and tries to re-enter SSVNetwork.registerValidator function. +contract FakeWhitelistingContract is ERC165 { + struct Cluster { + uint32 validatorCount; + uint64 networkFeeIndex; + uint64 index; + bool active; + uint256 balance; + } + + bytes private publicKey; + uint64[] private operatorIds; + bytes private sharesData; + uint256 private amount; + Cluster private clusterData; + + address private ssvContract; + + constructor(address _ssvContract) { + ssvContract = _ssvContract; + } + + function setRegisterValidatorData( + bytes calldata _publicKey, + uint64[] memory _operatorIds, + bytes calldata _sharesData, + uint256 _amount, + Cluster memory _cluserData + ) external { + publicKey = _publicKey; + operatorIds = _operatorIds; + sharesData = _sharesData; + amount = _amount; + clusterData = _cluserData; + } + + function supportsInterface(bytes4 interfaceId) public view virtual override returns (bool) { + return interfaceId == type(ISSVWhitelistingContract).interfaceId || super.supportsInterface(interfaceId); + } + + fallback() external { + bytes4 selector = bytes4(msg.data); + if (selector == ISSVWhitelistingContract.isWhitelisted.selector) { + // Encoding the registerValidator function selector and arguments + bytes memory data = abi.encodeWithSignature( + "registerValidator(bytes,uint64[],bytes,uint256,(uint32,uint64,uint64,bool,uint256))", + publicKey, + operatorIds, + sharesData, + amount, + clusterData + ); + // Making the low-level call + (bool success, bytes memory returnData) = ssvContract.call(data); + + // Handling the call response + if (!success) revert("Call failed or was reverted"); + } + } +} diff --git a/test/validators/whitelist-register.ts b/test/validators/whitelist-register.ts index 9194a4ec..9c18d06e 100644 --- a/test/validators/whitelist-register.ts +++ b/test/validators/whitelist-register.ts @@ -11,13 +11,15 @@ import { coldRegisterValidator, CONFIG, DEFAULT_OPERATOR_IDS, + publicClient, } from '../helpers/contract-helpers'; import { assertPostTxEvent } from '../helpers/utils/test'; import { trackGas, GasGroup, trackGasFromReceipt } from '../helpers/gas-usage'; +import { mine } from '@nomicfoundation/hardhat-toolbox-viem/network-helpers'; import { expect } from 'chai'; -let ssvNetwork: any, ssvViews: any, ssvToken: any, minDepositAmount: BigInt, mockWhitelistingContractAddress: any; +let ssvNetwork: any, ssvViews: any, ssvToken: any, minDepositAmount: BigInt; describe('Register Validator Tests', () => { beforeEach(async () => { @@ -244,7 +246,177 @@ describe('Register Validator Tests', () => { ).to.be.rejectedWith('CallerNotWhitelisted'); }); + it('Register using fake whitelisting contract reverts', async () => { + const fakeWhitelistingContract = await hre.viem.deployContract( + 'FakeWhitelistingContract', + [await ssvNetwork.address], + { + client: owners[0].client, + }, + ); + + // Set the whitelisting contract for operators 1,2,3,4 + await ssvNetwork.write.setOperatorsWhitelistingContract( + [DEFAULT_OPERATOR_IDS[4], await fakeWhitelistingContract.address], + { + account: owners[0].account, + }, + ); + await ssvToken.write.approve([ssvNetwork.address, minDepositAmount], { account: owners[3].account }); + + const pk = DataGenerator.publicKey(1); + const shares = await DataGenerator.shares(3, 1, 4); + + // set the 2nd registerValidator input data with a new validator public key + await fakeWhitelistingContract.write.setRegisterValidatorData([ + '0xa063fa1434f4ae9bb63488cd79e2f76dea59e0e2d6cdec7236c2bb49ffb37da37cb7966be74eca5a171f659fee7bc502', + [4, 5, 6, 7], + shares, + minDepositAmount, + { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0n, + active: true, + }, + ]); + + await expect( + ssvNetwork.write.registerValidator( + [ + pk, + [4, 5, 6, 7], + shares, + minDepositAmount, + { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0n, + active: true, + }, + ], + { account: owners[3].account }, + ), + ).to.be.rejectedWith('Call failed or was reverted'); // reverts in the fake whitelisting contract + }); + + it('Read-only reentrancy attack reverts', async () => { + // This test replicates a Read-only reentrancy attack, where a malicious whitelisting contract + // acts as an intermediary for a malicious contract that serves as an operator owner. + // It performs an attempt of withdrawing the operator earnings when the SSVNetwork contract + // still doesn't receive the funds, resulting in an inconsistent state of the contract. + // Expected result is a revert from the SSVNetwork contract because the + // ISSVWhitelistingContract.isWhitelisted function has the view modifier. + // The flow of the attack is the following: + // AttackerContract -> SSVNetwork.registerValidator() -> BadOperatorWhitelisting.fallback() + // -> BeneficiaryContract.withdrawOperatorEarnings() -> SSVNetwork.withdrawOperatorEarnings() + + const beneficiaryContract = await hre.viem.deployContract('BeneficiaryContract', [await ssvNetwork.address], { + client: owners[1].client, + }); + + const badOperatorWhitelistingContract = await hre.viem.deployContract( + 'BadOperatorWhitelistingContract', + [await beneficiaryContract.address], + { + client: owners[1].client, + }, + ); + + const attackerContract = await hre.viem.deployContract('AttackerContract', [await ssvNetwork.address], { + client: owners[1].client, + }); + + // BeneficiaryContract register the target operator + const { result: beneficiaryOperatorId } = await publicClient.simulateContract({ + address: await beneficiaryContract.address, + abi: beneficiaryContract.abi, + functionName: 'registerOperator', + account: owners[1].account, + }); + + await beneficiaryContract.write.registerOperator(); + await beneficiaryContract.write.setTargetOperatorId([beneficiaryOperatorId]); + + // Register a new operator, good owner + const { result: goodOperatorId } = await publicClient.simulateContract({ + address: await ssvNetwork.address, + abi: ssvNetwork.abi, + functionName: 'registerOperator', + args: ['0xabcd', CONFIG.minimalOperatorFee], + account: owners[0].account, + }); + + await ssvNetwork.write.registerOperator(['0xabcd', CONFIG.minimalOperatorFee]); + // Whitelist the new operator with the attacker contract + await ssvNetwork.write.setOperatorsWhitelistingContract( + [[goodOperatorId], await badOperatorWhitelistingContract.address], + { + account: owners[0].account, + }, + ); + + const goodUser = owners[1].account; + + // A good user calls registerValidator with operators: + // 1, 2, 3, beneficiaryOperatorId + await ssvToken.write.approve([ssvNetwork.address, minDepositAmount], { account: goodUser }); + + let pk = DataGenerator.publicKey(2); + let shares = await DataGenerator.shares(1, 1, 4); + + await ssvNetwork.write.registerValidator( + [ + pk, + [1, 2, 3, beneficiaryOperatorId], + shares, + minDepositAmount, + { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0n, + active: true, + }, + ], + { account: goodUser }, + ); + + // forward blocks so beneficiaryOperator generates revenue + await mine(10); + + // The attacker contract calls registerValidator with operators: + // 1, 2, beneficiaryOperatorId, goodOperatorId + const badUser = owners[3].account; + + pk = DataGenerator.publicKey(3); + shares = await DataGenerator.shares(3, 1, 4); + + // AttackerContract starts the attact + await expect( + attackerContract.write.startAttack( + [ + pk, + [1, 2, beneficiaryOperatorId, goodOperatorId], + shares, + minDepositAmount, + { + validatorCount: 0, + networkFeeIndex: 0, + index: 0, + balance: 0n, + active: true, + }, + ], + { account: badUser }, + ), + ).to.be.rejected; + }); + describe('Register using whitelisting contract', () => { + let mockWhitelistingContractAddress: any; beforeEach(async () => { // Whitelist whitelistedCaller using an external contract const mockWhitelistingContract = await hre.viem.deployContract(