Summary
The OperatorVault and CommunityVault contracts cannot update the rewardsController storage variable after deployment. This limitation prevents the vaults from claiming rewards if the Chainlink operator staking contract changes its rewardsController address.
Vulnerability Details
When a new OperatorVault (same applies for CommunityVault) instance is deployed using the OperatorVCS::addVault function, the OperatorVault::initialize function is triggered to set it up. One of the parameters passed is the address returned by stakeController.getRewardVault(), which is stored in the OperatorVault::rewardsController variable. This address points to the vault designated by the Chainlink operator contract (stakeController) for claiming rewards accrued by the vault.
The problem arises when the Chainlink operator staking contract (stakeController) updates the rewardVault address using the StakingPoolBase::setRewardVault function (see contract here). Existing OperatorVault instances will continue to reference the old rewardVault, rendering them unable to claim future rewards. The main drawback of the current implementation is that the address is not dynamically retrieved from the stakeController contract and/or cannot be updated through a setter function.
Below is the OperatorVCS::addVault function, which passes the StakeController::rewardVault address as a parameter to initialize the OperatorVault.
https://github.com/Cyfrin/2024-09-stakelink/blob/main/contracts/linkStaking/OperatorVCS.sol#L361-L379
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);
}
The StakeController::rewardVault address is then stored inside the OperatorVault::rewardsController storage variable.
* @notice Initializes contract
* @param _token address of LINK token
* @param _vaultController address of the strategy that controls this vault
* @param _stakeController address of Chainlink operator staking contract
* @param _rewardsController address of Chainlink staking rewards contract
* @param _pfAlertsController address of Chainlink price feed alrts controller
* @param _operator address of operator represented by this vault
* @param _rewardsReceiver address authorized to claim rewards from this vault
**/
function initialize(
address _token,
address _vaultController,
address _stakeController,
address _rewardsController,
address _pfAlertsController,
address _operator,
address _rewardsReceiver
) public reinitializer(3) {
if (vaultController == address(0)) {
__Vault_init(
_token,
_vaultController,
_stakeController,
@> _rewardsController
);
} else {
stakeController.migrate("");
stakeController = IStaking(_stakeController);
@> rewardsController = IStakingRewards(_rewardsController);
trackedTotalDeposits = SafeCast.toUint128(getTotalDeposits());
}
pfAlertsController = IPFAlertsController(_pfAlertsController);
rewardsReceiver = _rewardsReceiver;
if (operator == address(0) && _operator != address(0)) {
setOperator(_operator);
}
}
And is used to claim the rewards:
function updateDeposits(
uint256 _minRewards,
address _rewardsReceiver
) external onlyVaultController returns (uint256, uint256, uint256) {
uint256 principal = getPrincipalDeposits();
uint256 rewards = getRewards();
uint256 totalDeposits = principal + rewards;
int256 depositChange = int256(totalDeposits) - int256(uint256(trackedTotalDeposits));
uint256 opRewards;
if (depositChange > 0) {
opRewards =
(uint256(depositChange) *
IOperatorVCS(vaultController).operatorRewardPercentage()) /
10000;
unclaimedRewards += SafeCast.toUint128(opRewards);
trackedTotalDeposits = SafeCast.toUint128(totalDeposits);
}
if (_minRewards != 0 && rewards >= _minRewards) {
@> rewardsController.claimReward();
trackedTotalDeposits -= SafeCast.toUint128(rewards);
totalDeposits -= rewards;
token.safeTransfer(_rewardsReceiver, rewards);
}
return (totalDeposits, principal, opRewards);
}
Finally, there is no setter function to update the OperatorVault::rewardsController storage variable within either the OperatorVault or the Vault contract.
PoC
First add the missing setReward function to StakingMock:
function setRewardVault(address _newRewardVault) external {
rewardVault = _newRewardVault;
}
Add the following test to /test/linkStaking/operator-vcs.test.ts
In the test, we first create a new vault using the strategy.addVault function. Next, we update the rewardVault in the stakingController using the setRewardVault function. Finally, we verify that the vault still points to the old rewardVault address, which means it will not be able to claim rewards stored in the new rewardVault.
it.only("fails to update rewards controller", async () => {
const { accounts, adrs, stakingController, strategy } = await loadFixture(
deployFixture
);
await strategy.addVault(accounts[1], accounts[2], accounts[5]);
assert.equal((await strategy.getVaults()).length, 16);
let vault = await ethers.getContractAt(
"OperatorVault",
(
await strategy.getVaults()
)[15]
);
assert.equal(
await stakingController.getRewardVault(),
adrs.rewardsController
);
assert.equal(await vault.rewardsController(), adrs.rewardsController);
const newRewardVault = "0x1111111111111111111111111111111111111111";
await stakingController.setRewardVault(newRewardVault);
assert.equal(await stakingController.getRewardVault(), newRewardVault);
assert.equal(await vault.rewardsController(), adrs.rewardsController);
});
Impact
Vaults pointing to the old rewardVault address will be unable to claim future rewards.
Tools Used
Manual review
Recommendations
Either read rewardVault address dynamically from within the OperatorVault contract or add a setter function inside the OperatorVault contract to allow the owner to update the rewardVault address.
