Relevant GitHub Links
https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/linkStaking/OperatorVCS.sol#L360-L380
Summary
The addVault function in OperatorVCS.sol allows the contract owner to deploy new vaults without proper access restrictions beyond the onlyOwner modifier. If the owner account is compromised or if the onlyOwner modifier is incorrectly implemented, unauthorized users could add malicious vaults to the system, leading to potential fund theft or manipulation.
Vulnerability Details
File: contracts/linkStaking/OperatorVCS.sol
function addVault(
address _operator,
address _rewardsReceiver,
address _pfAlertsController
) external onlyOwner {
bytes memory data = abi.encodeWithSignature(
"initialize(address,address,address,address,address,address,address)",
address(token),
address(this),
address(stakeController),
stakeController.getRewardVault(),
_pfAlertsController,
_operator,
_rewardsReceiver
);
_deployVault(data);
vaultMapping[address(vaults[vaults.length - 1])] = true;
emit VaultAdded(_operator);
}
Centralization Risk: The addVault function is protected only by the onlyOwner modifier. This means that the contract owner has complete authority to add any number of new vaults, including potentially malicious ones.
Lack of Multi-Signature or Timelock: Without multi-signature requirements or a timelock mechanism, a single compromised or malicious owner account can endanger the entire system.
Impact
If an attacker gains control of the owner account or if the onlyOwner modifier is compromised Unauthorized vaults can be added, which can be programmed to siphon funds, manipulate rewards distribution, or perform other malicious activities. Allowing unauthorized vaults undermines the integrity and security of the entire staking system, potentially leading to significant financial losses. Such vulnerabilities can erode user trust, affecting the platform's reputation and user base.
PoC
test case demonstrating how an unauthorized user could exploit the addVault function if access controls are improperly implemented or bypassed
import { expect } from 'chai';
import { ethers } from 'hardhat';
describe('OperatorVCS Access Control Test', function () {
let operatorVCS: any;
let attacker: any;
let owner: any;
let token: any;
let stakeController: any;
beforeEach(async function () {
[owner, attacker] = await ethers.getSigners();
const TokenMock = await ethers.getContractFactory('ERC20Mock');
token = await TokenMock.deploy("Mock Token", "MTK", owner.address, ethers.utils.parseEther("10000"));
await token.deployed();
const StakeControllerMock = await ethers.getContractFactory('StakeControllerMock');
stakeController = await StakeControllerMock.deploy();
await stakeController.deployed();
const OperatorVCS = await ethers.getContractFactory('OperatorVCS');
operatorVCS = await OperatorVCS.deploy();
await operatorVCS.deployed();
await operatorVCS.initialize(
token.address,
owner.address,
stakeController.address,
owner.address,
[],
10000,
1000,
500,
owner.address
);
});
it('Should prevent non-owner from adding a vault', async function () {
await expect(
operatorVCS.connect(attacker).addVault(
attacker.address,
attacker.address,
attacker.address
)
).to.be.revertedWith("Ownable: caller is not the owner");
});
it('Owner can add a vault successfully', async function () {
await expect(
operatorVCS.connect(owner).addVault(
owner.address,
owner.address,
owner.address
)
).to.emit(operatorVCS, 'VaultAdded').withArgs(owner.address);
const vaultAddress = await operatorVCS.vaults(0);
expect(vaultAddress).to.properAddress;
});
});
Supporting Contracts:
ERC20Mock.sol// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
contract ERC20Mock is ERC20 {
constructor(string memory name, string memory symbol, address initialAccount, uint256 initialBalance) ERC20(name, symbol) {
_mint(initialAccount, initialBalance);
}
}
StakeControllerMock.sol
pragma solidity ^0.8.15;
contract StakeControllerMock {
address public rewardVault;
constructor() {
rewardVault = msg.sender;
}
function getRewardVault() external view returns (address) {
return rewardVault;
}
function unstake(uint256 _amount) external {
}
}
Setup: - Mock versions of ERC20, StakeController, and OperatorVCS are deployed.
The OperatorVCS contract is initialized with necessary parameters.
Attempted Exploit: - An unauthorized user (attacker) attempts to call addVault. - The test expects the transaction to revert with an "Ownable: caller is not the owner" error.
Expected Outcome: - Only the owner can successfully add a vault. - Unauthorized attempts are prevented, maintaining the contract's integrity.
Tools Used
Manual Code Review
Recommendations
Implement Timelock Mechanism: - Introduce a timelock for critical functions like addVault to allow users to react in case of malicious proposals.
import "@openzeppelin/contracts/governance/TimelockController.sol";
contract OperatorVCS is VaultControllerStrategy {
constructor(...) {
_setupRole(PROPOSER_ROLE, msg.sender);
_setupRole(EXECUTOR_ROLE, msg.sender);
_setupRole(CANCELLER_ROLE, msg.sender);
}
function addVault(...) external onlyProposer {
}
}
Restrict Vault Parameters: - Validate input parameters when adding new vaults to ensure they adhere to expected standards and prevent the deployment of malicious vaults.
function addVault(
address _operator,
address _rewardsReceiver,
address _pfAlertsController
) external onlyOwner {
require(_operator != address(0), "Invalid operator address");
require(_rewardsReceiver != address(0), "Invalid rewards receiver address");
require(_pfAlertsController != address(0), "Invalid alerts controller address");
}
