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

The stake amount stored in `Steaking:usersToStakes` is overwritten by the amount coming from the most recent `Steaking:stake` function call, rather than accumulating, when multiple staking operations are performed on the same beneficiary address

soupy

Summary

The stake amount recorded under Steaking:usersToStakes is overwritten by the most recent Steaking:stake function call, rather than being accumulated. This means that when multiple staking operations are performed on the same beneficiary address, the total stake amount is not correctly reflected, leading to incorrect records and fund loss for the user.

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

Vulnerability Details

The protocol allows user to stake ETH for themselves or for others using Steaking:stake function call during the staking period. The staked amount is recorded under Steaking:usersToStakes. However, if either user or other people stake additional ETH to the same beneficiary address, the additional stake amount is not added to the previously stored amount, instead being replaced by the staking amount from the most recent Steaking:stake function call.

@external
@payable
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)

Proof of Concept:
In test/Steaking.t.sol, add the following test:

function test_audit_incorrectStakeAmountForMultipleStakingOperations() public {
address user2 = makeAddr("user2");
uint256 amountToStake1 = 1 ether;
uint256 amountToStake2 = 0.5 ether;
vm.deal(user1, amountToStake1);
vm.deal(user2, amountToStake2);
​
vm.startPrank(user1);
steaking.stake{value: amountToStake1}(user2);
vm.stopPrank();
​
vm.startPrank(user2);
steaking.stake{value: amountToStake2}(user2);
vm.stopPrank();
​
uint256 expectedStakedAmount = amountToStake1 + amountToStake2;
uint256 actualStakedAmount = steaking.usersToStakes(user2);
console.log("expectedStakedAmount: ", expectedStakedAmount);
console.log("actualStakedAmount: ", actualStakedAmount);
​
assertEq(actualStakedAmount, expectedStakedAmount);
}

The test run will fail with console messages as below

Ran 1 test for test/Steaking.t.sol:SteakingTest
[FAIL. Reason: assertion failed: 500000000000000000 != 1500000000000000000] test_audit_incorrectStakeAmountForMultipleStakingOperations() (gas: 84549)
Logs:
expectedStakedAmount: 1500000000000000000
actualStakedAmount: 500000000000000000
​
...

This indicates that the top-up amount on second stake operation was not added to the existing staked amount but replaced as the new total stake amount for the same beneficiary address.

Impact

Incorrect record of staking amount and fund loss for the user/beneficary account

Tools Used

Manual review with forge test

Recommendations

Make amendment to the Steaking:stake function so that the staked amount for user/beneficary's address is correctly cumulated on multiple staking operations.

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.