Root + Impact
Description
-
There are ways surplus ETH could be sent, either by calling Game::claimThrone , which includes a require statement that msg.value could be more than claimFee permitting surplus ETH to be sent or directly sending ETH to the contract.
-
The protocol doesn't provide a way to withdraw stuck ETH sent to the contract , the surplus accounted ETH do not have a way to be removed, Hence it will be stuck forever.
function claimThrone() external payable gameNotEnded nonReentrant {
@> require(msg.value >= claimFee, "Game: Insufficient ETH sent to claim the throne.");
require(msg.sender == currentKing, "Game: You are already the king. No need to re-claim.");
uint256 sentAmount = msg.value;
uint256 previousKingPayout = 0;
uint256 currentPlatformFee = 0;
uint256 amountToPot = 0;
currentPlatformFee = (sentAmount * platformFeePercentage) / 100;
if (currentPlatformFee > (sentAmount - previousKingPayout)) {
currentPlatformFee = sentAmount - previousKingPayout;
}
platformFeesBalance = platformFeesBalance + currentPlatformFee;
amountToPot = sentAmount - currentPlatformFee;
pot = pot + amountToPot;
currentKing = msg.sender;
lastClaimTime = block.timestamp;
playerClaimCount[msg.sender] = playerClaimCount[msg.sender] + 1;
totalClaims = totalClaims + 1;
claimFee = claimFee + (claimFee * feeIncreasePercentage) / 100;
emit ThroneClaimed(
msg.sender,
sentAmount,
claimFee,
pot,
block.timestamp
);
}
Risk
Likelihood:
-
This happens when a user sends surplus ETH more than the claimFee required in Game::claimThrone , the Protocol has provides no way of sending the surplus back
-
This would also occur when a user sends ETH directly to the contract courtesy of receive function which enables it to receive the surplus ETH
Impact:
Proof of Concept
First Instance - User A sends 4 ether directly into the game contract, which ends up being lost forever
Second Instance invovles users sending more than current claim fee in a game round , assuming claim fee is set as 0.1 ether , someone could send more value than required
Test:
function testPeopleCanSendMoreThanClaimFee() public {
vm.startPrank(player1);
game.claimThrone{value: 2e18}();
}
Result:
Ran 1 test for test/Game.t.sol:GameTest
[PASS] testPeopleCanSendMoreThanClaimFee() (gas: 149826)
Traces:
[169726] GameTest::testPeopleCanSendMoreThanClaimFee()
├─ [0] VM::startPrank(player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84])
│ └─ ← [Return]
├─ [152621] Game::claimThrone{value: 2000000000000000000}()
│ ├─ emit ThroneClaimed(newKing: player1: [0x7026B763CBE7d4E72049EA67E89326432a50ef84], claimAmount: 2000000000000000000 [2e18], newClaimFee: 110000000000000000 [1.1e17], newPot: 1900000000000000000 [1.9e18], timestamp: 1)
│ └─ ← [Stop]
└─ ← [Stop]
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 23.72ms (2.08ms CPU time)
Recommended Mitigation
To tackle both instances, the protocol can send excess eth back to users and collect only the required amount
function claimThrone() external payable gameNotEnded nonReentrant {
require(msg.value >= claimFee, "Game: Insufficient ETH sent to claim the throne.");
require(msg.sender != currentKing, "Game: You are already the king. No need to re-claim.");
- uint256 sentAmount = msg.value;
+ uint256 excessAmount = msg.value - claimFee;
+ uint256 sentAmount = claimFee;
uint256 previousKingPayout = 0;
uint256 currentPlatformFee = 0;
uint256 amountToPot = 0;
// Calculate platform fee
currentPlatformFee = (sentAmount * platformFeePercentage) / 100;
// Defensive check to ensure platformFee doesn't exceed available amount after previousKingPayout
if (currentPlatformFee > (sentAmount - previousKingPayout)) {
currentPlatformFee = sentAmount - previousKingPayout;
}
platformFeesBalance = platformFeesBalance + currentPlatformFee;
// Remaining amount goes to the pot
amountToPot = sentAmount - currentPlatformFee;
pot = pot + amountToPot;
// Update game state
currentKing = msg.sender;
lastClaimTime = block.timestamp;
playerClaimCount[msg.sender] = playerClaimCount[msg.sender] + 1;
totalClaims = totalClaims + 1;
// Increase the claim fee for the next player
claimFee = claimFee + (claimFee * feeIncreasePercentage) / 100;
+ if (excessAmount > 0) {
+ (bool success, ) = msg.sender.call{value: excessAmount}("");
+ require(success, "ETH refund failed");
+ }
emit ThroneClaimed(msg.sender, sentAmount, claimFee, pot, block.timestamp);
}
