Skip to content

Commit

Permalink
add reentrancy tests
Browse files Browse the repository at this point in the history
  • Loading branch information
mtabasco committed May 13, 2024
1 parent 38b7165 commit 11b4e67
Show file tree
Hide file tree
Showing 5 changed files with 327 additions and 1 deletion.
25 changes: 25 additions & 0 deletions contracts/test/mocks/AttackerWhitelistingContract.sol
Original file line number Diff line number Diff line change
@@ -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);
}
}
33 changes: 33 additions & 0 deletions contracts/test/mocks/BadOperatorWhitelistingContract.sol
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
29 changes: 29 additions & 0 deletions contracts/test/mocks/BeneficiaryContract.sol
Original file line number Diff line number Diff line change
@@ -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);
}
}
67 changes: 67 additions & 0 deletions contracts/test/mocks/FakeWhitelistingContract.sol
Original file line number Diff line number Diff line change
@@ -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");
}
}
}
174 changes: 173 additions & 1 deletion test/validators/whitelist-register.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit 11b4e67

Please sign in to comment.