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

`Steaking:stake` fails to check existing users balance, resulting in a loss of funds for the user

brutalclowney

Summary

Steaking:stake updates the users staked balance in the protocol with the msg.value of the function call. This interaction fails to check if the user has any existing value in the contract.

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 <@ // fails to check for existing value of users balance in array
self.totalAmountStaked += msg.value
​
log Staked(msg.sender, msg.value, _onBehalfOf)

Vulnerability Details (POC)

Since the value of the deposit isn't added to the existing value of the array, a users deposited total can only ever be equal to the last deposited amount.

Add the console contract to the Steaking.t.sol test suite:

- import {Test} from "lib/forge-std/src/Test.sol";
+ import {Test, console} from "lib/forge-std/src/Test.sol";

Then add the following test :

function testDepositGriefsUser() public {
// deposit a large amount from user1
vm.deal(user1, 1 ether);
uint256 dealAmount = 1 ether;
_stake(user1, dealAmount, user1);
​
// log user1s balance in the protocol
uint256 user1BalanceBefore = steaking.usersToStakes(user1);
console.log("user1 balance before", user1BalanceBefore);
​
// user1 wants to stake more
vm.deal(user1, 1 ether);
uint256 dealAmount2 = 0.5 ether;
_stake(user1, dealAmount2, user1);
​
// log user1s balance in the protocol
uint256 user1BalanceAfter = steaking.usersToStakes(user1);
console.log("user1 balance after", user1BalanceAfter);
​
// assert user1s balance has decreased
assertEq(steaking.usersToStakes(user1), dealAmount2);
}

the output from the test will return

Ran 1 test for test/Steaking.t.sol:SteakingTest
[PASS] testDepositGriefsUser() (gas: 85897)
Logs:
user1 balance before 1000000000000000000
user1 balance after 500000000000000000

Confirming that the users balance in the protocol is now only 0.5 ether.

Impact

Any user who receives more than one deposit will lose the value of their previous deposits.

Tools Used

Foundry, Manual review

Recommendations

A simple way of ammending this issue is to add the msg.value to the users balance in the deposit function.

- self.usersToStakes[_onBehalfOf] = msg.value
self.totalAmountStaked += msg.value
+ self.usersToStakes[_onBehalfOf] += msg.value
self.totalAmountStaked += 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.