Last Man Standing
First Flight #45
Beginner FriendlyFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Impact: medium
Likelihood: high
Invalid

Discrepancies in contract balance due to potential direct ether transfers

soupy

Summary

The balance returned by getContractBalance may not align with the NatSpec expectation of reflecting the contract's actual balance (i.e., pot plus platform fees, accounting for any pending payouts), as the contract's receive function allows it to accept direct Ether transfers unrelated to game play, potentially causing discrepancies.

Description

The getContractBalance function is documented via NatSpec to return the contract's current balance, which should reflect the sum of the pot and platform fees, accounting for any pending payouts:

/**
<@@> * @dev Returns the current balance of the contract (should match the pot plus platform fees unless payouts are pending).
*/
function getContractBalance() public view returns (uint256) {
<@@> return address(this).balance;
}

However, the contract includes an explicitly receive function, which allows it to accept direct Ether transfers. If funds are sent directly to the contract (i.e., outside the intended gameplay flow), they will be included in address(this).balance, inflating the reported balance. As a result, returning address(this).balance may not accurately represent the game play related funds, making the implementation inconsistent with the NatSpec comment.

Risk

Likelihood:

  • Given that the receive function is implemented and the contract can accept unsolicited Ether transfers, it is likely that at some point, funds unrelated to gameplay will be sent to the contract. This can happen unintentionally (e.g., mistaken transfers) or through manual interactions with the contract, especially if the address is known.

Impact:

  • If external Ether is sent directly to the contract via the receive function, the balance returned by getContractBalance will include funds unrelated to gameplay.

  • This can lead to:

    • Misleading balance reporting, where the returned value does not accurately reflect the pot and platform fees as expected.

    • Incorrect assumptions by users, especially if external systems/front-end UI rely on getContractBalance for decision-making on game state.

    • Potential issues in audits or accounting, as untracked or unexpected funds inflate the balance, creating confusion or mismatches in financial reports.

Proof of Concept

In test/Game.t.sol, add the following test:

function test_audit_contractBalanceMismatch() public {
console2.log("contractBalanceAtStart: ", game.getContractBalance());
​
// player 1 claim throne, contract starts to have game related fund
vm.prank(player1);
game.claimThrone{value: INITIAL_CLAIM_FEE}();
​
// the actual amount coming from the game player
uint256 manualTrackGameBalance = game.pot() + game.platformFeesBalance() + game.pendingWinnings(player1);
console2.log("manualTrackGameBalance: ", manualTrackGameBalance);
​
// some player(player2) mistakenly transfer 0.5 ether to the game contract
vm.prank(player2);
(bool success, ) = payable(address(game)).call{value: 0.5 ether}("");
require(success, "direct fund transfer failed");
​
uint256 gameContractBalance = game.getContractBalance();
console2.log("gameContractBalance: ", gameContractBalance);
​
// this assertion shows that the game contract balance is not tally to game related fund
assert(gameContractBalance > manualTrackGameBalance);
}

In terminal, run forge test --match-test test_audit_contractBalanceMismatch -vvv will generate the following results:

$ forge test --match-test test_audit_contractBalanceMismatch -vvv
...
​
Ran 1 test for test/Game.t.sol:GameTest
[PASS] test_audit_contractBalanceMismatch() (gas: 173993)
Logs:
contractBalanceAtStart: 0
manualTrackGameBalance: 100000000000000000
gameContractBalance: 600000000000000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 749.38µs (98.58µs CPU time)
​
Ran 1 test suite in 121.27ms (749.38µs CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

The test showed that the enquired game contract balance was not tally to the manual track game balance, indicating that the getContractBalance function doesn't not reflect the rightful actual game play related fund.

Recommended Mitigation

To ensure the accuracy of getContractBalance and align it with the NatSpec expectation (i.e., pot plus platform fees, excluding unrelated funds), consider the followings:

  1. amend the return output

function getContractBalance() public view returns (uint256) {
- return address(this).balance;
+ return pot + platformFeesBalance;
}

2.remove receive function to avoid any unexpected transfers

3.log the direct deposits separately for auditing purpose

Updates

Appeal created

inallhonesty Lead Judge about 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

Direct ETH transfers - User mistake

There is no reason for a user to directly send ETH or anything to this contract. Basic user mistake, info, invalid according to CH Docs.

Support

FAQs

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