First Flight #22: Steaking
First Flight #22
Beginner FriendlyFoundryDeFi
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Users can lose funds when staking multiple times on behalf of the same address

0xiceman

Summary

Link: https://github.com/Cyfrin/2024-08-steaking/blob/87e81eb617f3bca3dfbd7c6d4c90bdf1c988ebb0/steaking-contracts/src/Steaking.vy#L73

In the stake() function of the Steaking contract, the usersToStakes[_onBehalfOf] value is overwritten each time a user stakes ETH, rather than incremented. Therefore, every new stake effectively resets the user’s total staked amount to the latest value, erasing any prior contributions.

Vulnerability Details

When a user stakes ETH, the following line in the stake() function overwrites the existing amount (which could be != 0) in the usersToStakes mapping:

self.usersToStakes[_onBehalfOf] = msg.value

Instead of adding the new amount to the existing stake, this line replaces the previous amount entirely. Therefore, every new stake effectively resets the user’s total staked amount to the latest value, erasing any prior contributions.

Impact

Due to the overwriting of the staked amount in the usersToStakes mapping, users who stake multiple times on behalf of the same address will only be able to unstake the most recent staked amount, not the total of all their stakes. This effectively locks up any prior contributions, preventing users from recovering the full amount of ETH they originally staked.

After the staking period ends, users are supposed to convert their staked ETH into WETH and deposit it into the vault. However, because only the most recent stake is recorded, users will not be able to deposit the actual total amount of ETH they staked. Instead, they will only be able to deposit the amount from their last transaction, resulting in a discrepancy between the actual ETH staked and the WETH deposited into the vault.

So the user's funds are lost and remain locked in the contract.

PoC:

Place the following test into steaking-contracts/test/Steaking.t.sol;

function testStakeOverwrites() public {
// Step 1: Initial stake
uint256 dealAmount1 = steaking.getMinimumStakingAmount(); // First stake amount
_stake(user1, dealAmount1, user1);
​
// Verify initial stake
uint256 userStakedAmount1 = steaking.usersToStakes(user1);
uint256 totalAmountStaked1 = steaking.totalAmountStaked();
​
assertEq(userStakedAmount1, dealAmount1);
assertEq(totalAmountStaked1, dealAmount1);
​
// Step 2: Stake again
uint256 dealAmount2 = steaking.getMinimumStakingAmount(); // Second stake amount
_stake(user1, dealAmount2, user1);
​
// Verify that the user's stake is overwritten and not added
uint256 userStakedAmount2 = steaking.usersToStakes(user1);
uint256 totalAmountStaked2 = steaking.totalAmountStaked();
​
// Expected: userStakedAmount is now equal to dealAmount2, not dealAmount1 + dealAmount2
assertEq(userStakedAmount2, dealAmount2);
// totalAmountStaked will still account for both the first and second stake
assertEq(totalAmountStaked2, dealAmount1 + dealAmount2);
}

Tools Used

Manual Review

Recommendations

Modify the logic in the stake() function to accumulate the user's stake rather than overwriting it. Replace the line that overwrites the stake amount with the following:

self.usersToStakes[_onBehalfOf] += msg.value
Updates

Lead Judging Commences

inallhonesty Lead Judge 10 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Steaking::stake overwrites the msg.value into storage

Support

FAQs

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