Description:
The steaking contract lacks a fallback function to handle ETH sent directly to the steaking contract.
Impact:
Users may lose their funds if they mistakenly send ETH directly to the steaking contract.
Proof of Concept:
User initial balance is 8 ether.
User just sends 2 ether directly to the steaking contract.
User loses his 2 ether within the steaking contract since it doesn't have a fallback function to redirect the ETH to the stake function.
Loss of funds is evident since it is not recorded in the userToStakes mapping and the totalAmountStaked variable.
Code
Place the following into Steaking.t.sol
function testUserSendsETHDirect() public {
vm.deal(user1, 8 ether);
vm.startPrank(user1);
(bool sent, bytes memory data) = address(steaking).call{value: 2 ether}("");
vm.stopPrank();
uint256 userStakedAmount = steaking.usersToStakes(user1);
uint256 totalAmountStaked = steaking.totalAmountStaked();
console2.log("This is the total staked Amount of User1::", userStakedAmount);
console2.log("This is the total amount staked in the contract::", totalAmountStaked);
require(sent, "Failed to send Ether directly to contract");
assert(userStakedAmount == 2 ether);
assert(totalAmountStaked == 2 ether);
assert(address(user1).balance == 6 ether);
}
Here are the logs that were shown.
[27733] SteakingTest::testUserSendsETHDirect()
├─ [0] VM::deal(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 8000000000000000000 [8e18])
│ └─ ← [Return]
├─ [0] VM::startPrank(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Return]
├─ [117] Steaking::fallback{value: 2000000000000000000}()
│ └─ ← [Revert] EvmError: Revert
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [2360] Steaking::usersToStakes(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 0
├─ [2223] Steaking::totalAmountStaked() [staticcall]
│ └─ ← [Return] 0
├─ [0] console::log("This is the total staked Amount of User1::", 0) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("This is the total amount staked in the contract::", 0) [staticcall]
│ └─ ← [Stop]
└─ ← [Revert] revert: Failed to send Ether directly to contract
As we can see, the steaking contract lacks a fallback function to handle the direct ETH transfer from a user. The ETH tragically gets lost in the steaking contract since it cannot be recorded in the userToStakes mapping which records 0 and also the totalAmountStaked variable which also records 0.
Recommended Mitigation:
Implement a fallback function to manage direct ETH transfers.
Here is the recommended fix.
+ @external
+ @payable
+ def __default__():
+ self._stake(msg.sender)
+ @internal
+ @payable
+ def _stake(_onBehalfOf: address):
+ """
+ @notice Internal function to handle staking logic.
+ """
+ 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)
- 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)
+ @external
+ @payable
+ def stake(_onBehalfOf: address):
+ """
+ @notice External function that calls the internal staking logic.
+ """
+ self._stake(_onBehalfOf)
The stake function is split into an internal _stake function and an external stake function. The internal function can be called directly by the __default__ function.
This changes may make this test that was shared in the Proof of Concept section pass successfully as it is evident in the logs.
[72443] SteakingTest::testUserSendsETHDirect()
├─ [0] VM::deal(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], 8000000000000000000 [8e18])
│ └─ ← [Return]
├─ [0] VM::startPrank(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Return]
├─ [48646] Steaking::fallback{value: 2000000000000000000}()
│ ├─ emit Staked(by: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF], amount: 2000000000000000000 [2e18], onBehalfOf: user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF])
│ └─ ← [Stop]
├─ [0] VM::stopPrank()
│ └─ ← [Return]
├─ [360] Steaking::usersToStakes(user1: [0x29E3b139f4393aDda86303fcdAa35F60Bb7092bF]) [staticcall]
│ └─ ← [Return] 2000000000000000000 [2e18]
├─ [223] Steaking::totalAmountStaked() [staticcall]
│ └─ ← [Return] 2000000000000000000 [2e18]
├─ [0] console::log("This is the total staked Amount of User1::", 2000000000000000000 [2e18]) [staticcall]
│ └─ ← [Stop]
├─ [0] console::log("This is the total amount staked in the contract::", 2000000000000000000 [2e18]) [staticcall]
│ └─ ← [Stop]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 23.94ms (2.84ms CPU time)
