Liquid Staking

Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Submission Details
Severity: high
Invalid

Race condition in vault removal function allow removing an active vault in `PriorityPool.sol`

Summary

In the OperatorVCS contract, the queueVaultRemoval and removeVault functions are responsible for handling the removal of vaults. A race condition exists due to the contract's reliance on external state conditions when determining whether a vault should be queued for removal. Specifically, a vault can be queued for removal based on the status of an operator, but if the operator’s status changes before the vault is actually removed, it can result in unintended behavior such as removing an active vault. This vulnerability can be exploited to disrupt the proper functioning of vault management within the contract.

Vulnerability Details

The vault removal process in OperatorVCS has two steps: queuing the vault for removal in the queueVaultRemoval function and subsequently removing it in removeVault. The removal process relies on external state conditions such as the operator's status in the Chainlink staking contract. However, a race condition can occur if the status of the operator (e.g., whether the operator is removed from the staking contract) changes between the queuing and removal steps. This could allow an attacker or even normal users to exploit the system by triggering vault removal while the operator is still active or has changed status.

function queueVaultRemoval(uint256 _index) external {
address vault = address(vaults[_index]);
if (!IVault(vault).isRemoved()) revert OperatorNotRemoved();
for (uint256 i = 0; i < vaultsToRemove.length; ++i) {
if (vaultsToRemove[i] == vault) revert VaultRemovalAlreadyQueued();
}
vaultsToRemove.push(address(vaults[_index]));
// Attempt to remove vault immediately if claim period is active
if (_index < globalVaultState.depositIndex) {
uint256 group = _index % globalVaultState.numVaultGroups;
uint256;
groups[0] = group;
fundFlowController.updateOperatorVaultGroupAccounting(groups);
if (vaults[_index].claimPeriodActive()) {
removeVault(vaultsToRemove.length - 1);
}
}
}

Race condition:

  1. The function queueVaultRemoval queues the vault for removal based on the external IVault(vault).isRemoved() status. This status check determines whether the vault is eligible for removal based on the operator's status in the staking contract.

  2. However, the operator’s status can change after the vault is queued but before it is actually removed. The contract relies on the state of vaultsToRemove and doesn’t re-check the operator's status during the removeVault step.

  3. If the operator is reactivated or the vault becomes active again, the vault could still be removed, causing unintended behavior and potentially disrupting the staking system.

PoC scenario:

  1. Assume there is an active vault associated with an operator that becomes temporarily inactive (e.g., removed from the staking contract).

  2. A user or attacker calls queueVaultRemoval to add the vault to the removal queue. The operator’s status is checked and the vault is queued.

  3. Before removeVault is called, the operator’s status changes, and the vault becomes active again.

  4. When removeVault is executed, the vault is removed despite being active, potentially leading to incorrect behavior such as loss of funds or disruption in staking operations.

Here is a test that can be run using Hardhat to demonstrate the race condition:

const { expect } = require("chai");
describe("OperatorVCS - Vault Removal Race Condition", function () {
let operatorVCS, vaultMock, stakingPoolMock, owner, operator;
beforeEach(async function () {
const [deployer, _operator, _owner] = await ethers.getSigners();
owner = _owner;
operator = _operator;
// Mock contracts for vault and staking pool
const VaultMock = await ethers.getContractFactory("VaultMock");
vaultMock = await VaultMock.deploy();
await vaultMock.deployed();
const StakingPoolMock = await ethers.getContractFactory("StakingPoolMock");
stakingPoolMock = await StakingPoolMock.deploy();
await stakingPoolMock.deployed();
// Deploy the OperatorVCS contract
const OperatorVCS = await ethers.getContractFactory("OperatorVCS");
operatorVCS = await OperatorVCS.deploy();
await operatorVCS.deployed();
// Initialize the contract
await operatorVCS.initialize(
vaultMock.address,
stakingPoolMock.address,
owner.address,
vaultMock.address,
[],
10000, // maxDepositSizeBP
5000, // vaultMaxDeposits
500, // operatorRewardPercentage
owner.address
);
});
it("should remove an active vault due to race condition", async function () {
// Step 1: Queue vault for removal while operator is removed
await vaultMock.setRemoved(true);
await operatorVCS.queueVaultRemoval(0); // index 0 is queued for removal
// Step 2: Operator status changes and vault is no longer removed
await vaultMock.setRemoved(false); // operator is now active again
// Step 3: Remove vault, race condition occurs
await operatorVCS.removeVault(0);
// Expect vault to still be removed even though operator is active again
const vaultsAfterRemoval = await operatorVCS.getVaultRemovalQueue();
expect(vaultsAfterRemoval.length).to.equal(0); // Vault was removed
});
});

The test will pass, showing that the vault was removed even though the operator was reactivated after being queued for removal, demonstrating the race condition.

Impact

The race condition allows for the unintended removal of vaults that have become active again after being queued for removal. This could result in:

  • Active vaults being removed, causing operational disruptions.

  • Potential loss of staking rewards for operators and users.

  • Unpredictable behavior in the vault management process.

Tools Used

Manual review.

Recommendations

  1. In the removeVault function, add an additional check to ensure that the vault is still in a removed state before allowing removal. This will prevent vaults that have been reactivated from being removed.

    Example:

function removeVault(uint256 _queueIndex) public {
address vault = vaultsToRemove[_queueIndex];
// Recheck the vault's removed status before removing
if (!IVault(vault).isRemoved()) revert VaultStillActive();
// Continue with removal process...
}
  1. Instead of relying on external states that can change between function calls, consider bundling the removal process into a single atomic transaction where the vault is checked and removed in one step, reducing the risk of state changes between calls.

Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.