Summary
The FjordStaking::unstakeAll iterates over the entire activeDeposits array without an upper bound. This leads to gas exhaustion if the array is very large, causing the transaction to fail.
Link: https://github.com/Cyfrin/2024-08-fjord/blob/0312fa9dca29fa7ed9fc432fdcd05545b736575d/src/FjordStaking.sol#L570C5-L607C6
Vulnerability Details
The for loop in the unstakeAll function does not have an upper bound, which means it will attempt to process all active deposits in a single transaction. If a user has a large number of active deposits, the gas required to execute the function may exceed the block gas limit, resulting in a failed transaction.
function unstakeAll()
external
checkEpochRollover
redeemPendingRewards
returns (uint256 totalStakedAmount)
{
uint256[] memory activeDeposits = getActiveDeposits(msg.sender);
if (activeDeposits.length == 0) revert NoActiveDeposit();
@> for (uint16 i = 0; i < activeDeposits.length; i++) {
uint16 epoch = uint16(activeDeposits[i]);
DepositReceipt storage dr = deposits[msg.sender][epoch];
if (dr.epoch == 0 || currentEpoch - epoch <= lockCycle) continue;
totalStakedAmount += dr.staked;
if (dr.vestedStaked == 0) {
delete deposits[msg.sender][epoch];
_activeDeposits[msg.sender].remove(epoch);
} else {
dr.staked = 0;
}
}
totalStaked -= totalStakedAmount;
userData[msg.sender].totalStaked -= totalStakedAmount;
fjordToken.transfer(msg.sender, totalStakedAmount);
points.onUnstaked(msg.sender, totalStakedAmount);
emit UnstakedAll(
msg.sender, totalStakedAmount, activeDeposits, getActiveDeposits(msg.sender)
);
}
Impact
Add this test in the unstakeAll.t.sol and
run: forge test --match-test test_Unstake_All_GasExaustion -vv
function test_Unstake_All_GasExaustion() public {
uint256 initialGas = gasleft();
for (uint256 i = 0; i < 195; i++) {
_addRewardAndEpochRollover(2 ether, 1);
assertEq(fjordStaking.currentEpoch(), i + 2);
vm.prank(alice);
fjordStaking.stake(1 ether);
}
_addRewardAndEpochRollover(2 ether, 7);
vm.prank(alice);
fjordStaking.unstakeAll();
uint256 finalGas = gasleft();
uint256 gasUsed = initialGas - finalGas;
console2.log("gasUsed", gasUsed);
uint256 GAS_LIMIT = 30_000_000;
assert(gasUsed > GAS_LIMIT);
}
Ran 1 test for test/unit/unstakeAll.t.sol:UnstakeAll_Unit_Test
[PASS] test_Unstake_All_GasExaustion() (gas: 24340800)
Logs:
gasUsed 30427659
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 629.64ms (42.46ms CPU time)
The test shows that for an active deposit array equal to 195 the block gas limit is passed.
Users with a large number of active deposits are unable to unstake their tokens due to gas exhaustion (Dos).
Tools Used
Manual review
Recommendations
Implement an upper limit to handle unstaking for the large deposits.
