Liquid Staking
Stakelink
DeFiHardhatOracle
50,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Integer Truncation Risk in Vault Withdrawal Logic

juggernaut63

Summary

The withdraw function in the VaultDepositController contract performs an unsafe downcast from uint256 to uint128 when updating the totalDepositRoom of a vault group. This can lead to data truncation if the value being cast exceeds the maximum value that a uint128 can hold, resulting in incorrect accounting of deposit room and financial discrepancies.

Vulnerability Details

The issue arises from the unsafe casting of totalWithdrawn from uint256 to uint128 without checking if the value fits within the uint128 range.

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/base/VaultControllerStrategy.sol#L161

function withdraw(uint256 _amount, bytes calldata _data) external {
if (!fundFlowController.claimPeriodActive() || _amount > totalUnbonded)
revert InsufficientTokensUnbonded();
​
GlobalVaultState memory globalState = globalVaultState;
uint64[] memory vaultIds = abi.decode(_data, (uint64[]));
VaultGroup memory group = vaultGroups[globalState.curUnbondedVaultGroup];
​
// withdrawals must continue with the vault they left off at during the previous call
if (vaultIds[0] != group.withdrawalIndex) revert InvalidVaultIds();
​
uint256 toWithdraw = _amount;
uint256 unbondedRemaining = totalUnbonded;
(uint256 minDeposits, ) = getVaultDepositLimits();
​
for (uint256 i = 0; i < vaultIds.length; ++i) {
// vault must be a member of the current unbonded group
if (vaultIds[i] % globalState.numVaultGroups != globalState.curUnbondedVaultGroup)
revert InvalidVaultIds();
​
group.withdrawalIndex = uint64(vaultIds[i]);
IVault vault = vaults[vaultIds[i]];
uint256 deposits = vault.getPrincipalDeposits();
​
if (deposits != 0 && vault.claimPeriodActive() && !vault.isRemoved()) {
if (toWithdraw > deposits) {
vault.withdraw(deposits);
unbondedRemaining -= deposits;
toWithdraw -= deposits;
} else if (deposits - toWithdraw > 0 && deposits - toWithdraw < minDeposits) {
// cannot leave a vault with less than minimum deposits
vault.withdraw(deposits);
unbondedRemaining -= deposits;
break;
} else {
vault.withdraw(toWithdraw);
unbondedRemaining -= toWithdraw;
break;
}
}
}
​
uint256 totalWithdrawn = totalUnbonded - unbondedRemaining;
​
token.safeTransfer(msg.sender, totalWithdrawn);
​
totalDeposits -= totalWithdrawn;
totalPrincipalDeposits -= totalWithdrawn;
totalUnbonded = unbondedRemaining;
​
@=> group.totalDepositRoom += uint128(totalWithdrawn);
vaultGroups[globalVaultState.curUnbondedVaultGroup] = group;
}

Scenario

  1. Context:

    • User A wants to withdraw a large amount of tokens from the vault.

    • The total number of tokens to be withdrawn is 2^130, which exceeds the limit of uint128.

  2. Withdrawal Process:

    • The withdraw function is called with the withdrawal amount of 2^130.

    • totalWithdrawn is calculated as 2^130.

  3. Downcasting:

    • When totalWithdrawn (2^130) is cast to uint128, it exceeds the maximum limit of uint128 (2^128 - 1).

    • As a result, the value of totalWithdrawn is truncated to a smaller value, such as 0, because only the lower half of the bits are taken.

  4. Impact:

    • group.totalDepositRoom does not reflect the correct amount due to the truncated value.

    • User A does not receive the correct number of tokens, or may be shown the wrong deposit room.

Impact

The truncation can lead to incorrect accounting of the totalDepositRoom, causing discrepancies in the vault's financial records.

Tools Used

Maual review

Recommendations

use OpenZeppelin's SafeCast library to safely downcast integers.

Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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