Beginner FriendlyFoundryDeFi
100 EXP
View results
Submission Details
Severity: high
Valid

Staking ETH multiple times by a user will cause the previously deposited ETH to be stuck in the Steaking contract forever.

Summary

A user stakes ETH using the function Steaking::stake as mentioned below.

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_S_ADDRESS_ZERO
@=> self.usersToStakes[_onBehalfOf] = msg.value
self.totalAmountStaked += msg.value
log Staked(msg.sender, msg.value, _onBehalfOf)

This function maintains a mapping Steaking::usersToStakes, which maps the address of users to the amount staked. When a user stakes ETH, the mapping is updated to the amount of ETH sent in the value . Because of this, the Steaking::usersToStakes mapping discards the amount already staked previously by the user.

As a result, after the Steaking::depositIntoVault becomes live, the user will be able to deposit only the latest staked amount by them into the vault. This limitation arises because Steaking::depositIntoVault allows users to deposit only the amount recorded in the Steaking::usersToShares mapping, which is incorrect and will lead to only partial amount of the total staked ETH to be deposited into the vault and hence causing the remaining staked ETH of the user to be stuck in the Steaking contract forever.

Vulnerability Details

Impact

This vulnerability has a critical impact on the protocol. Users will be unable to deposit their entire staked ETH once Steaking::depositIntoVault is live, causing all previously staked ETH to remain trapped in the Steaking contract permanently. Therefore, the severity of this vulnerability is assessed as high.

Tools Used

manual code review

Proof of code

We prove the incorrectness of the hashmap using the below test function.

Paste the following function in the Steaking.t.sol

function testUserLosesPreviouslyDepositedFundsOnDepositingAgain() public{
// 1. User deposits 8eth.
// 2. User deposits 1eth.
// 3. Total user balance should be 8 + 1 = 9 eth.
uint256 totalAmountDeposited;
vm.deal(user1, 9 ether);
//User deposits 8 ether
vm.startPrank(user1);
steaking.stake{value: 8 ether}(address(user1));
vm.stopPrank();
totalAmountDeposited += 8 ether;
//now user deposits another 1 eth
vm.startPrank(user1);
steaking.stake{value : 1 ether}(address(user1));
vm.stopPrank();
totalAmountDeposited += 1 ether;
//Retrieving the amount of funds stored in the hashmap for user1.
uint256 amountStoredInHashmap = steaking.usersToStakes(address(user1));
// The total amount deposited will be > the amount shown in the hashmap.
console.log("The actual amount that user deposited :", totalAmountDeposited);
console.log("The amount according to the hashmap :", amountStoredInHashmap);
//This will not allow the user to deposit all of their funds from the contract to the vault.
assert(totalAmountDeposited > amountStoredInHashmap);
}

Recommendations

In order to mitigate this vulnerability, change the following lines. This change will ensure that the amount sent is added to the user's previously staked amount:

@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_S_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.