fix: cap jackpot payout to the pot size at the winner's own commit (prevents delayed-reveal pot sniping)#11
Open
eltociear wants to merge 1 commit into
Conversation
revealShot() can legally be called anywhere from commitBlock+2 up to
commitBlock+MAX_REVEAL_DELAY (256 blocks) later, and once the reveal
block's hash is fixed the win/lose outcome only depends on values that
are readable off-chain (secret is known to the caller; nonce/
playerNonces are plain storage, "private" only hides them from other
Solidity contracts, not from external inspection). A player who commits,
waits two blocks, and checks off-chain that they've already won can
simply hold their pending shot open and only call revealShot() once the
pot has been inflated by other players committing in the meantime,
collecting a payout far larger than the pot that existed when their own
win was actually determined.
Fix: snapshot the pot size right after each shot's own commit
(potAtCommit) and cap _handleWin's payout to min(currentPot, potAtCommit).
Pot growth from shots committed after the winner's own commit is excluded
from the payout and rolls over into the next round instead of being
deducted-to-zero or handed to this winner.
Also fixes a related bug in the same function: shot.amount was read from
the storage-pointer `shot` *after* `delete pendingShots[msg.sender]`, so
ShotRevealed's amount argument always logged 0 regardless of the actual
shot size. Needed to capture values out of `shot` before the delete
anyway for potAtCommit, so fixed both in the same pass.
Verified with `npx hardhat compile` (compiles clean, evm target paris).
Could not run the existing contract test suite in this environment —
test/contracts/EthShot.test.js hits a pre-existing ESM/CJS import error
unrelated to this change (`import { ethers } from 'hardhat'` against a
CommonJS build).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #10.
_handleWincurrently pays outWIN_PERCENTAGE_BPof the livecurrentPotat the momentrevealShotis actually called — which can legally be up to 256 blocks after the win was already determined (see #10 for the full writeup). This lets a player who already knows off-chain that their pending shot wins simply sit on it while other players' shots inflate the pot, then reveal for a much bigger payout than the pot that existed when their win was actually locked in.Fix
potAtCommittoPendingShot, snapshottingcurrentPotright after each shot's own contribution is added (bothcommitShotandcommitFirstShot)._handleWinnow takespotAtCommitand pays outmin(currentPot, potAtCommit) * WIN_PERCENTAGE_BP / BASIS_POINTSinstead of the raw live pot. Only the awarded amount is deducted fromcurrentPot(currentPot -= potAmount, notcurrentPot = 0), so any excess pot growth from later shots correctly rolls over into the next round instead of being wiped or handed to this winner.revealShotnow readsshot.amountandshot.potAtCommitinto locals beforedelete pendingShots[msg.sender]. This also fixes a pre-existing bug in the same lines:shotis a storage pointer, so the old code'semit ShotRevealed(msg.sender, shot.amount, won)— which ran after the delete — always loggedamount = 0, sincedeletezeroes the storage thatshotpoints at.Verification
npx hardhat compile— compiles clean (evm target paris, no warnings from the new code).test/contracts/EthShot.test.jsin my environment — it fails on a pre-existing, unrelated ESM/CJS interop error (import { ethers } from 'hardhat'against Hardhat's CommonJS build), not something introduced by this change. Happy to add a dedicated test for the capped-payout behavior if you point me at the right test setup for this repo (or once that import issue is sorted).No changes to
canWin, the RNG source, reveal timing, or any other behavior — this only changes how much of the pot a win is allowed to draw from.