Description:
The line self.usersToStakes[_onBehalfOf] = msg.value overwrites the user's previous stake with the new value instead of adding to the existing stake. If a user stakes multiple times, only the most recent staked amount will be recorded, effectively erasing their previous staked contributions.
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.totalAmountStaked += msg.value
log Staked(msg.sender, msg.value, _onBehalfOf)
Impact:
This issue directly affects the accuracy of user staked balances and the overall integrity of the staking mechanism. It is a critical issue because it involves the potential loss of user staked funds, making it valid and severe.
Proof of Concept:
User has 16 ether as his balance.
User stakes 8 ether into the steaking contract.
User stakes another 8 ether into the steaking contract.
User expects to have staked 16 ether in the steaking contract.
User expects to have (16 * 1000) points accumulated over his staked ETH in the steaking contract.
User ends up getting (8 * 1000) points accumulated over his staked ETH due to wrong implementation of mapping.
The steaking contract recorded that the user has just staked just only 8 ETH instead of 16 ETH due to wrong implementation of mapping.
Code
Place the following into Steaking.t.sol.
function testUserMultipleStakeAttemptsAreRecorded() public {
vm.deal(user1, 16 ether);
vm.startPrank(user1);
steaking.stake{value: 8 ether}(user1);
steaking.stake{value: 8 ether}(user1);
vm.stopPrank();
uint256 userStakedAmount = steaking.usersToStakes(user1);
uint256 totalAmountStaked = steaking.totalAmountStaked();
console2.log("This is how much the user Staked::", userStakedAmount);
console2.log("---------------------------------------------------------");
console2.log("This is the total Amount Staked::", totalAmountStaked);
console2.log("---------------------------------------------------------");
assertEq(userStakedAmount, 16 ether);
assertEq(totalAmountStaked, 16 ether);
}
Here are the logs that were shown:
[0] console::log("This is how much the user Staked::", 8000000000000000000 [8e18]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("---------------------------------------------------------") [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("This is the total Amount Staked::", 16000000000000000000 [1.6e19]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("---------------------------------------------------------") [staticcall]
│ └─ ← [Stop]
├─ [0] VM::assertEq(8000000000000000000 [8e18], 16000000000000000000 [1.6e19]) [staticcall]
│ └─ ← [Revert] assertion failed: 8000000000000000000 != 16000000000000000000
└─ ← [Revert] assertion failed: 8000000000000000000 != 16000000000000000000
Here we can see an error where the mapping has recorded that the user had just staked just 8 ETH instead of 16 ETH. As the first assertion failed.
Recommended Mitigation:
Instead of overwriting, the function should add the new stake to the existing one:
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)
