Last Man Standing
First Flight #45
Beginner FriendlyFoundrySolidity
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

`getContractBalance` Function is Redundant and Ambiguous

superdevfavour

getContractBalance Function is Redundant and Ambiguous, Offering Poor Utility and Potential for Misinterpretation

Description

  • Normal Behavior: A well-designed view function in a smart contract should provide a clear, unambiguous value that offers utility to developers. It should have a self-explanatory name and a comment that accurately describes a consistent return value.

  • The Issue: The function is documented with the comment: Returns the current balance of the contract (should match the pot plus platform fees unless payouts are pending). While the parenthetical clause makes this statement technically correct, it creates several problems:

    1. Redundancy: The function's implementation, return address(this).balance;, is redundant. Any external caller or off-chain script can already get this exact value with a standard balance check on the contract's address. The function adds no new capability.

    2. Ambiguity: The function's value has two different interpretations depending on the contract's state (whether payouts are pending or not). A developer using this function must perform additional checks to understand what the returned value actually represents, defeating the purpose of a simple view function.

    3. Low Utility: Because of the above points, the function provides very little practical value and can be considered a "code smell." It complicates the contract's interface without providing a clear benefit.

/**
* @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;
}

Risk

Likelihood: HIGH

  • The function will always be redundant, and its meaning will always be state-dependent, causing potential confusion for any developer who tries to use it.

Impact: LOW

  • This issue does not directly risk funds. However, it represents poor code quality and a confusing ABI. This can lead to subtle bugs in off-chain software, wasted developer time, and incorrect assumptions about the contract's financial state during audits or integrations.

Proof of Concept

  • Deploy the Game contract.

  • Using an external script or tool (like Ethers.js or Hardhat), get the contract's balance directly: provider.getBalance(contract.address).

  • Call the getContractBalance() function on the contract.

  • Observe: The values returned in Step 2 and Step 3 are identical, proving the function is redundant.

  • Analyze: To validate the state based on the comment, a developer must read the return value of this function and check if pendingWinnings contains any non-zero balances. This multi-step process to understand a single value indicates a poorly designed function interface.

function testContractBalance() public {
vm.prank(player1);
game.claimThrone{value: INITIAL_CLAIM_FEE}();
vm.startPrank(player2);
game.claimThrone{value: INITIAL_CLAIM_FEE * 2}();
vm.warp(block.timestamp + GRACE_PERIOD + 1);
game.declareWinner();
uint256 pot = game.pot();
uint256 platformFeesBalance = game.platformFeesBalance();
​
uint256 supposedBalanceReading = pot + platformFeesBalance;
uint256 actualBalanceReading = game.getContractBalance();
​
assert(supposedBalanceReading != actualBalanceReading);
}
​

Recommended Mitigation

The contract's interface should be improved by removing ambiguity and providing clear, purpose-built functions.

Recommendation: Deprecate and remove the confusing getContractBalance function. Replace it with a new function that provides the value described in the first part of the comment, as this is a genuinely useful piece of information.

  1. Remove the getContractBalance function entirely.

  2. Add a new, clearly named function:

+ /**
+ * @dev Returns the sum of funds currently in the active prize pot and held as platform fees.
+ * This represents the total value staked in the current, ongoing game round.
+ */
+ function getActiveGameFunds() public view returns (uint256) {
+ return pot + platformFeesBalance;
+ }

This change removes the redundant and confusing function and replaces it with one that has a clear name, a clear purpose, and provides real utility to developers, thereby improving the overall quality and security of the contract's interface.

Updates

Appeal created

inallhonesty Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Game::getContractBalance doesn't behave as it should

Support

FAQs

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

Give us feedback!