Submission Details
Severity:
`OperatorVault::updateDeposits` does not correctly update `trackedTotalDeposits`, causing discrepancies
Summary
trackedTotalDeposits is not reduced by opRewards so it will cause trackedTotalDeposits have more than what it actually deposited to ChainLink Stake Contract
Vulnerability Details
When calling updateDeposits, it will calculate depositChange and if positive it will continue to calculate respective rewards and deposits:
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);
}
.
.
.
The problem arises when opRewards is added to unclaimedRewards but not deducted from totalDeposits. As a result, when updating trackedTotalDeposits, the state variable will have more than it should.
Proof of Code:
make new file Base.t.sol in test folder:
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;
import {Test} from "../lib/forge-std/src/Test.sol";
import {console2} from "../lib/forge-std/src/console2.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import {ERC677} from "contracts/core/tokens/base/ERC677.sol";
import {StakingPool} from "contracts/core/StakingPool.sol";
import {StrategyMock} from "contracts/core/test/StrategyMock.sol";
import {SDLPoolMock} from "contracts/core/test/SDLPoolMock.sol";
import {PriorityPool} from "contracts/core/priorityPool/PriorityPool.sol";
import {WithdrawalPool} from "contracts/core/priorityPool/WithdrawalPool.sol";
contract Base is Test {
ERC677 token;
SDLPoolMock sdlPool;
StakingPool stakingPool;
StrategyMock strategy;
PriorityPool priorityPool;
WithdrawalPool withdrawalPool;
address owner = makeAddr("owner");
address alice = makeAddr("alice");
address bob = makeAddr("bob");
address charlie = makeAddr("charlie");
uint256 constant TOKEN_AMOUNT = 10000 * 1e18;
uint256 constant DEAD_SHARES = 1000;
uint128 constant MIN_DEPOSIT = 10 * 1e18;
uint128 constant MAX_DEPOSIT = 1000 * 1e18;
uint256 constant UNUSED_DEPOSIT = 1000 * 1e18;
uint256 constant MIN_WITHDRAW = 10 * 1e18;
uint64 constant MIN_TIME_BETWEEN_WITHDRAWALS = 86400;
function setUp() public virtual {
StakingPool.Fee[] memory fees = new StakingPool.Fee[]();
fees[0] = StakingPool.Fee({receiver: address(0), basisPoints: 0});
vm.startPrank(owner);
token = new ERC677("Chainlink", "LINK", 1e9);
sdlPool = new SDLPoolMock();
StakingPool stakingPoolImpl = new StakingPool();
StrategyMock strategyImpl = new StrategyMock();
PriorityPool priorityPoolImpl = new PriorityPool();
WithdrawalPool withdrawalPoolImpl = new WithdrawalPool();
ERC1967Proxy stakingPoolProxy = new ERC1967Proxy(
address(stakingPoolImpl),
abi.encodeCall(StakingPool.initialize, (address(token), "Staked LINK", "stLINK", fees, UNUSED_DEPOSIT))
);
stakingPool = StakingPool(address(stakingPoolProxy));
ERC1967Proxy strategyProxy = new ERC1967Proxy(
address(strategyImpl),
abi.encodeCall(StrategyMock.initialize, (address(token), address(stakingPool), MAX_DEPOSIT, MIN_DEPOSIT))
);
strategy = StrategyMock(address(strategyProxy));
ERC1967Proxy priorityPoolProxy = new ERC1967Proxy(
address(priorityPoolImpl),
abi.encodeCall(
PriorityPool.initialize,
(address(token), address(stakingPool), address(sdlPool), MIN_DEPOSIT, MAX_DEPOSIT)
)
);
priorityPool = PriorityPool(address(priorityPoolProxy));
ERC1967Proxy withdrawalPoolProxy = new ERC1967Proxy(
address(withdrawalPoolImpl),
abi.encodeCall(
WithdrawalPool.initialize,
(
address(token),
address(stakingPool),
address(priorityPool),
MIN_WITHDRAW,
MIN_TIME_BETWEEN_WITHDRAWALS
)
)
);
withdrawalPool = WithdrawalPool(address(withdrawalPoolProxy));
stakingPool.addStrategy(address(strategy));
stakingPool.setPriorityPool(address(priorityPool));
stakingPool.setRebaseController(address(0));
priorityPool.setDistributionOracle(address(0));
priorityPool.setWithdrawalPool(address(withdrawalPool));
token.approve(address(priorityPool), type(uint256).max);
// deposit dead shares
priorityPool.deposit(DEAD_SHARES, false, new bytes[](1));
vm.stopPrank();
}
}
then add OperatorVault.t.sol in the same folder:
// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;
import {console2} from "../lib/forge-std/src/console2.sol";
import {Base} from "./Base.t.sol";
import {OperatorVault} from "contracts/linkStaking/OperatorVault.sol";
import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
import {StakingMock} from "contracts/linkStaking/test/StakingMock.sol";
import {StakingRewardsMock} from "contracts/linkStaking/test/StakingRewardsMock.sol";
import {PFAlertsControllerMock} from "contracts/linkStaking/test/PFAlertsControllerMock.sol";
import {OperatorVCSMock} from "contracts/linkStaking/test/OperatorVCSMock.sol";
contract OperatorVaultTest is Base {
OperatorVault operatorVault;
StakingMock staking;
StakingRewardsMock stakingReward;
PFAlertsControllerMock pfAlert;
OperatorVCSMock operatorVCS;
// helper address
address vault = makeAddr("vault");
address operator = makeAddr("operator");
address rewardReceiver = makeAddr("rewardReceiver");
function setUp() public override {
super.setUp();
staking = new StakingMock(address(token), vault, 10e18, 100e18, 1000e18, 0, 0);
stakingReward = new StakingRewardsMock(address(token));
// set operator reward percentage to 10% (1000 bps)
operatorVCS = new OperatorVCSMock(address(token), 1000, 0);
OperatorVault operatorVaultImpl = new OperatorVault();
ERC1967Proxy operatorVaultProxy = new ERC1967Proxy(
address(operatorVaultImpl),
abi.encodeCall(
OperatorVault.initialize,
(
address(token),
address(operatorVCS),
address(staking),
address(stakingReward),
address(pfAlert),
operator,
rewardReceiver
)
)
);
operatorVault = OperatorVault(address(operatorVaultProxy));
operatorVCS.addVault(address(operatorVault));
}
function test_POC_UpdateDepositsDiscrepancy() public {
uint256 strategyDepositAmount = 100e18;
uint256 stakingRewardAmount = 10e18;
vm.prank(address(operatorVCS));
token.approve(address(operatorVault), type(uint256).max);
// simulate operatorVCS deposit of 100e18 tokens
vm.startPrank(alice);
deal(address(token), alice, strategyDepositAmount);
token.approve(address(operatorVCS), type(uint256).max);
operatorVCS.deposit(strategyDepositAmount);
assertEq(operatorVault.trackedTotalDeposits(), strategyDepositAmount);
// simulate staking reward already have stakingRewardAmount then set reward
deal(address(token), address(stakingReward), stakingRewardAmount);
stakingReward.setReward(address(operatorVault), stakingRewardAmount);
// update deposits, min reward 1 so it will claim rewards
operatorVCS.updateDeposits(1, address(operatorVault));
// getUnclaimedRewards() result needs to deducted with token.balanceOf(address(operatorVault)) to get the correct unclaimed rewards
// https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/OperatorVault.sol#L144-L146
uint256 opUnclaimedReward = operatorVault.getUnclaimedRewards() - token.balanceOf(address(operatorVault));
// assert
uint256 totalTokenOnStateVar = operatorVault.trackedTotalDeposits() + opUnclaimedReward;
uint256 totalTokenOnStakingContract = token.balanceOf(address(staking));
assertEq(totalTokenOnStateVar, totalTokenOnStakingContract);
}
}
run the following command forge test --mt test_POC_UpdateDepositsDiscrepancy -vvvv
the result would FAIL:
Failing tests:
Encountered 1 failing test in test/OperatorVault.t.sol:OperatorVaultTest
[FAIL. Reason: assertion failed: 101000000000000000000 != 100000000000000000000] test_POC_UpdateDepositsDiscrepancy() (gas: 744949)
Impact
trackedTotalDeposits should track the amount of tokens it deposit into the ChainLink Stake Contract, if this is not accurate then there would be financial losses for the protocol.
The inflated value of trackedTotalDeposits can affect the accuracy of deposit and withdraw function, also the amount of distribution reward especially when checking getPendingRewards (e.g. when OperatorVCS::getPendingFee is called by StakingPool) because the depositChange value is deducted from trackedTotalDeposits.
Tools Used
foundry
Recommendations
change the lofic of updateDeposits function so it deduct the opRewards:
diff --git a/contracts/linkStaking/OperatorVault.sol b/contracts/linkStaking/OperatorVault.sol
index 64ccb0a..2e926a5 100644
--- a/contracts/linkStaking/OperatorVault.sol
+++ b/contracts/linkStaking/OperatorVault.sol
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);
+ totalDeposits -= 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);
}
run the command again forge test --mt test_POC_UpdateDepositsDiscrepancy -vvvv
now it would accurately and the test PASSED:
├─ [0] VM::assertEq(100000000000000000000 [1e20], 100000000000000000000 [1e20]) [staticcall]
│ └─ ← [Return]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.74ms (1.16ms CPU time)
Updates
Prize pool breakdown
Total prize
50,000 USDC
nSLOC
2,53020 USDC / LOC
42,000 USDC
4,250 USDC
3,750 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.