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

`Steaking::stake` overrides the staked amount for given users cause fund loss for them

abhishekthakur

Summary

The stake function overwrites the previous stake instead of adding to it, preventing users from increasing their stake.

Vulnerability Details

Relavant Links - 2024-08-steaking/steaking-contracts/src/Steaking.vy at main · Cyfrin/2024-08-steaking (github.com)

def stake(_onBehalfOf: address):
"""
@notice Allows users to stake ETH for themselves or any other user within the staking period.
@param _onBehalfOf The address to stake on behalf of.
"""
assert not self._hasStakingPeriodEnded(), STEAK__STAKING_PERIOD_ENDED
assert msg.value >= MIN_STAKE_AMOUNT, STEAK__INSUFFICIENT_STAKE_AMOUNT
assert _onBehalfOf != ADDRESS_ZERO, STEAK__ADDRESS_ZERO
​
@> self.usersToStakes[_onBehalfOf] = msg.value
self.totalAmountStaked += msg.value
​
log Staked(msg.sender, msg.value, _onBehalfOf)

If you see the highlighted line, userToStakes mapping is meant to keep track of user staked amount. However, it's overriding the previous value when called.
This poses a high risk for users, as users funded will be lost.

Consider a scenerio, when user stakes 1 ether for first time.

2 ether for second time and 0.6 ether for 3rd time. Due to current logic user staked balance will show as 0.6 ether only, rather 3.6 ether.
User looses 3 ether in this case.

POC

In existing test add follwoing test

function testStakeOverridePrevAmount() public {
_stake(user1, 1 ether, user1);
_stake(user1, 2 ether, user1);
_stake(user1, 0.6 ether, user1);
​
uint256 userStakedAmount = steaking.usersToStakes(user1);
uint256 totalAmountInStakingPool = steaking.totalAmountStaked();
​
assertEq(totalAmountInStakingPool, userStakedAmount);
}

Then run it on terminal. It will fail with showing the actual values,

3.6 ether vs 0.6 ether.
Total staked in pool is updated correctly while user staked balance is showing wrong.

Impact

Loss of funds for user

Tools Used

Manual Review, Foundry

Recommendations

Update the the function as given below to fix this error.

def stake(_onBehalfOf: address):
"""
@notice Allows users to stake ETH for themselves or any other user within the staking period.
@param _onBehalfOf The address to stake on behalf of.
"""
assert not self._hasStakingPeriodEnded(), STEAK__STAKING_PERIOD_ENDED
assert msg.value >= MIN_STAKE_AMOUNT, STEAK__INSUFFICIENT_STAKE_AMOUNT
assert _onBehalfOf != ADDRESS_ZERO, STEAK__ADDRESS_ZERO
​
- self.usersToStakes[_onBehalfOf] = msg.value
+ self.usersToStakes[_onBehalfOf] += msg.value
self.totalAmountStaked += msg.value
​
log Staked(msg.sender, msg.value, _onBehalfOf)
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.