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

Incorrect amount of staked assets may be assigned when staking on behalf of the user

arinthal

Summary

Staking ETH on behalf of another user overwrites the previous value for the user receiving the staking assets.

Vulnerability Details

The Steaking::stake function allows staking raw ETH on behalf of another user. When user (user2) stakes assets on behalf of another user (user1), who had previously staked assets for himself, the previously stored value in the usersToStakes variable for user1 will be overwritten with the new value from msg.value from the transaction sent by user2.

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)

Impact

Assets previously staked by user1 will no longer be tracked by the usersToStakes variable, though they will still be included in the totalAmountStaked variable. As a result, user1 will be unable to withdraw their originally staked assets and will only have access to the amount staked on their behalf by user2. In an edge case, user2 could reduce allocation of user1 to 0.5 ETH, which is the minimum stake amount.

Proof of Concept

  1. user1 stakes 5 ETH, and this amount is assigned for him in the usersToStakes variable

  2. user2 then stakes 0.5 ETH on behalf of user1, which overwrites the previous value in the usersToStakes variable. As a result, user1 is now only able to withdraw 0.5 ETH, instead of the originally staked 5 ETH.

Proof of Code

Add the following code to the Steaking.t.sol file within the SteakingTest contract.

function testStakeOnBehalfOf() public {
address user2 = makeAddr("user2");
​
uint256 dealAmount1 = 5 ether;
uint256 dealAmount2 = steaking.getMinimumStakingAmount();
​
_stake(user1, dealAmount1, user1);
_stake(user2, dealAmount2, user1);
​
uint256 user1StakedAmount = steaking.usersToStakes(user1);
uint256 user2StakedAmount = steaking.usersToStakes(user2);
​
uint256 totalAmountStaked = steaking.totalAmountStaked();
​
assertEq(user1StakedAmount, dealAmount2);
assertEq(user2StakedAmount, 0);
assertEq(totalAmountStaked, dealAmount1 + dealAmount2);
}

Tools Used

  • Manual Review

  • Foundry

Recommended mitigation

Instead of setting a new value of the staked ETH, the amount form the msg.value variable should be added to the previously staked ETH for that specific user.

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.