Summary
During our evaluation of the CommunityVCS contract, a critical issue was identified regarding the adjustment of vault deposit limits when maxDeposits changes. Specifically, the logic responsible for updating the deposit rooms of vault groups was found to be flawed. The issue is evidenced by the provided test results, indicating that the expected deposit room values were not correctly updated.
Vulnerability Details
The issue originates in the deposit function within the CommunityVCS.sol contract. Below is the relevant code segment:
function deposit(uint256 _amount, bytes calldata _data) external override onlyStakingPool {
(, uint256 maxDeposits) = getVaultDepositLimits();
if (maxDeposits > vaultMaxDeposits) {
uint256 diff = maxDeposits - vaultMaxDeposits;
uint256 totalVaults = globalVaultState.depositIndex;
uint256 numVaultGroups = globalVaultState.numVaultGroups;
uint256 vaultsPerGroup = totalVaults / numVaultGroups;
uint256 remainder = totalVaults % numVaultGroups;
for (uint256 i = 0; i < numVaultGroups; ++i) uint256 numVaults = vaultsPerGroup;
if (i < remainder) {
numVaults += 1;
}
vaultGroups[i].totalDepositRoom += uint128(numVaults * diff);
}
vaultMaxDeposits = maxDeposits;
}
if (vaultDepositController == address(0)) revert VaultDepositControllerNotSet();
(bool success,) = vaultDepositController.delegatecall(
abi.encodeWithSelector(VaultDepositController.deposit.selector, _amount, _data)
);
if (!success) revert DepositFailed();
}
In this code, the deposit function checks whether the maxDeposits value has increased. If so, it adjusts the totalDepositRoom for each vault group. However, the logic for updating these deposit rooms is flawed, leading to incorrect assignment of deposit room values.
POC
it('should adjust vault deposit limits correctly when maxDeposits changes', async function () {
const { adrs, strategy, token, stakingController } = await loadFixture(deployFixture);
await strategy.deposit(toEther(50), encodeVaults([]));
console.log('Initial deposit of 50 ether');
console.log('Balance of stakingController:', fromEther(await token.balanceOf(adrs.stakingController)));
console.log('Total principal deposits:', fromEther(await strategy.totalPrincipalDeposits()));
assert.equal(fromEther(await token.balanceOf(adrs.stakingController)), 50);
assert.equal(fromEther(await strategy.totalPrincipalDeposits()), 50);
await stakingController.setDepositLimits(toEther(10), toEther(200));
console.log('Set new deposit limits: min 10 ether, max 200 ether');
await strategy.deposit(toEther(150), encodeVaults([]));
console.log('Deposit of 150 ether');
console.log('Balance of stakingController:', fromEther(await token.balanceOf(adrs.stakingController)));
console.log('Total principal deposits:', fromEther(await strategy.totalPrincipalDeposits()));
assert.equal(fromEther(await token.balanceOf(adrs.stakingController)), 200);
assert.equal(fromEther(await strategy.totalPrincipalDeposits()), 200);
const vaultMaxDeposits = await strategy.vaultMaxDeposits();
console.log('Vault max deposits:', fromEther(vaultMaxDeposits));
assert.equal(fromEther(vaultMaxDeposits), 200);
const globalVaultState = await strategy.globalVaultState();
const totalVaults = globalVaultState.depositIndex;
const numVaultGroups = globalVaultState.numVaultGroups;
const vaultsPerGroup = totalVaults / numVaultGroups;
const remainder = totalVaults % numVaultGroups;
console.log('Global vault state:', globalVaultState);
console.log('Total vaults:', totalVaults);
console.log('Number of vault groups:', numVaultGroups);
console.log('Vaults per group:', vaultsPerGroup);
console.log('Remainder:', remainder);
for (let i = 0; i numVaultGroups; ++i) {
const vaultGroup = await strategy.vaultGroups(i);
console.log(`Vault Group ${i} before update:`, vaultGroup);
let expectedDepositRoom = Number(vaultsPerGroup) * 150;
if (i < remainder) {
expectedDepositRoom += 150;
}
console.log(`Vault Group ${i}: expected ${expectedDepositRoom}, actual ${fromEther(vaultGroup.totalDepositRoom)}`);
assert.equal(Number(fromEther(vaultGroup.totalDepositRoom)), expectedDepositRoom);
console.log(`Vault Group ${i} after update:``, vaultGroup);
}
});
Output:
Initial deposit of 50 ether
Balance of stakingController: 50
Total principal deposits: 50
Set new deposit limits: min 10 ether, max 200 ether
Deposit of 150 ether
Balance of stakingController: 200
Total principal deposits: 200
Vault max deposits: 200
Global vault state: Result(4) [ 5n, 0n, 0n, 1n ]
Total vaults: 1n
Number of vault groups: 5n
Vaults per group: 0n
Remainder: 1n
Vault Group 0 before update: Result(2) [ 0n, 0n ]
Vault Group 0: expected 150, actual 0
1) should adjust vault deposit limits correctly when maxDeposits changes
5 passing (958ms) 1 failing
CommunityVCS should adjust vault deposit limits correctly when maxDeposits changes:
AssertionError: expected +0 to equal 150
expected - actual
-0 +150
The test indicates that Vault Group 0's deposit room was expected to be adjusted to 150 ether but remained at 0. This suggests that the logic within the deposit function does not correctly handle updates to the deposit room values when maxDeposits changes.
Impact
Impact: Incorrect adjustments to vault deposit limits can lead to improper fund allocation and potential protocol inefficiencies. This can affect the overall reliability and efficiency of the protocol.
Likelihood of Exploitation: Medium, as the incorrect adjustments primarily affect protocol operations and efficiency rather than posing a direct security risk
Tools Used
Recommendations
Error Handling: Add comprehensive error handling and logging within the deposit function to detect and respond to unusual conditions or errors during deposit room adjustments.
Revert States: Ensure that in case of failures, the contract can correctly revert to a stable state without impacting the consistency of deposit limits across vault groups.
