Rock Paper Scissors
First Flight #38
Beginner FriendlySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Valid

Withdraw function does not account for funds sent directly to contract, resulting in funds being irretrievable

0xnelli

Summary

The RockPaperScissors::withdrawFees functions does not account for funds sent directly to the contract. As the contract has receive() function the contract can directly receive funds, however, these funds are not added to accumlatedFees and therefore become stuck in the contract and cannot be withdrawn by the owner or sender resulting in a loss of funds.

Vulnerability Details

Funds are lost whenever funds are sent directly RockPaperScissors contract, as there is no function to withdraw these funds. The withdrawFees function will only withdraw a maximum amount of accumulatedFees, however, this variable is not updated when funds are directly transferred to the contract.

PoC

function testWithdrawFeesAfterDirectTransfer() public {
// First create and complete a game to generate fees
gameId = createAndJoinGame();
​
// Play a full game to generate fees
playTurn(
gameId,
RockPaperScissors.Move.Paper,
RockPaperScissors.Move.Rock
);
​
playTurn(
gameId,
RockPaperScissors.Move.Rock,
RockPaperScissors.Move.Scissors
);
​
playTurn(
gameId,
RockPaperScissors.Move.Rock,
RockPaperScissors.Move.Rock
);
​
// Calculate expected fees
uint256 totalBet = BET_AMOUNT * 2;
uint256 expectedFees = (totalBet * 10) / 100; // 10% fee
​
// Verify accumulated fees
assertEq(game.accumulatedFees(), expectedFees);
​
// Send funds directly to contract
vm.deal(playerA, 10 ether);
vm.prank(playerA);
address(game).call{value: 5 ether}("");
assert(address(game).balance >= 5 ether);
​
// Withdraw fees
uint256 adminBalanceBefore = address(this).balance;
​
vm.expectEmit(true, false, false, true);
emit FeeWithdrawn(address(this), expectedFees);
​
game.withdrawFees(0); // 0 means withdraw all
​
// Verify admin received fees
assertEq(address(this).balance - adminBalanceBefore, expectedFees);
assertEq(game.accumulatedFees(), 0);
assert(address(game).balance >= 5 ether);
console2.log(address(game).balance);
}

The PoC above demonstrates that a supposed maximal withdrawal does not affect the tokens directly transferred to the contract and 5 ether remains.

Impact

Funds are lost whenever tokens are directly transferred to the RockPaperScissors contract.

Tools Used

Manual review

Recommendations

Add a withdrawMax function with onlyOwner permission that withdraws the entire holdings of the contract address(this).balance.

Updates

Appeal created

m3dython Lead Judge about 2 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Orphaned ETH due to Unrestricted receive() or Canceled Game

ETH sent directly to the contract via the receive function or after a canceled game becomes permanently locked

m3dython Lead Judge about 2 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Orphaned ETH due to Unrestricted receive() or Canceled Game

ETH sent directly to the contract via the receive function or after a canceled game becomes permanently locked

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.