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

[H-1] Anyone can change the Staking amount of any other person to arbitrary value

nilay27

Description:

In Steaking::stake it can be called by any user on behalf of anyone and the balance of the person on whose behalf the stake was made can be changed!

@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
​
# @audit high anyone can set the value to 0.5!
# use self.usersToStakes[_onBehalfOf] += msg.value
self.usersToStakes[_onBehalfOf] = msg.value
self.totalAmountStaked += msg.value
​
log Staked(msg.sender, msg.value, _onBehalfOf)

Impact:

This is a critical vulnerability that can lead to loss of funds of users as their entire balance can be reset to the MIN_STAKE_AMOUNT that is expected by the contract.

Proof of Concept:

Add the following test to Steaking.t.sol:

function testAnyoneCanResetStakingAmount() public {
address user2 = makeAddr("user2");
​
// let user1 stake 10 ether on behalf of himself!
_stake(user1, 10 ether, user1);
console.log("amount staked by user1 before", steaking.usersToStakes(user1));
​
// let user2 set the staking amount of user1 to min amount i.e 0.5 ether!
_stake(user2, 0.5 ether, user1);
console.log("amount staked by user1 after ", steaking.usersToStakes(user1));
}

To run the test:

forge test --mt testAnyoneCanResetStakingAmount -vvv

The output of the balance can clearly be seen in the log:

amount staked by user1 before 10000000000000000000
amount staked by user1 after 500000000000000000

So even though user1 staked 10 ETH on his behalf, user2 was able to change his balance to the MIN_STAKE_AMOUNT i.e 0.5 ETH

Moreover, there is no sweep function in the contract, so even the owner can not help the user and the remaining 9.5 eth is locked in the contract forever!

Recommended Mitigation:

Change the stake function to following:

@external
@payable
def stake(_onBehalfOf: address):
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 about 1 year 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.