Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
|
others are not issues |
| import { ReputationToken } from '../ReputationToken.sol'; | ||
| import { Zoltar } from '../Zoltar.sol'; | ||
| import { ISecurityPool } from './interfaces/ISecurityPool.sol'; | ||
| import { YesNoMarkets } from './YesNoMarkets.sol'; |
There was a problem hiding this comment.
This file doesn't exist.
-- GLM 5
There was a problem hiding this comment.
There was a problem hiding this comment.
That isn't part of this branch, and this branch is based on main not that branch. Was this PR supposed to be based on that branch instead?
There was a problem hiding this comment.
GLM is the LLM I have been using of late.
|
|
||
| // todo, this should be calculated against to actual nonDecisionThreshold, not the one set at the start. The actual can be lower than the games treshold but never above | ||
| function claimDepositForWinning(uint256 depositIndex, YesNoMarkets.Outcome outcome) public returns (address depositor, uint256 amountToWithdraw) { | ||
| require(msg.sender == address(securityPool) || msg.sender == address(securityPool.securityPoolForker()), 'Only Security Pool can withdraw'); |
There was a problem hiding this comment.
securityPoolForker doesn't exist on ISecurityPool.
-- GLM 5
There was a problem hiding this comment.
|
|
||
| contract EscalationGameFactory { | ||
| function deployEscalationGame(uint256 startBond, uint256 _nonDecisionThreshold) external returns (EscalationGame) { | ||
| ISecurityPool securityPool = ISecurityPool(payable(msg.sender)); |
There was a problem hiding this comment.
Should we do something to ensure that this can only be called by "valid" security pools, or is this a situation where we anyone can claim to be a security pool and that isn't a problem?
-- GLM 5
There was a problem hiding this comment.
anyone can deploy escalation game, that shouldn't be an issue. Its just a tool to crowdsource funds for zoltar reach some decision
| if (attritionCost >= maxCost) return escalationTimeLength; | ||
|
|
||
| // binary search | ||
| for (uint256 iteration = 0; iteration < 64; iteration++) { |
There was a problem hiding this comment.
Is 64 iterations enough?
-- GLM 5
There was a problem hiding this comment.
I don't know. I also don't know if we will use this approach in future
There was a problem hiding this comment.
Probably worth a CONSIDER or TODO comment then, to ensure it doesn't survive long term like this if we haven't really thought about it.
There was a problem hiding this comment.
there's todo above the function
| require(startingTime == 0, 'already started'); | ||
| require(_nonDecisionThreshold > _startBond, 'threshold must exceed start bond'); | ||
| require(_startBond > 0, 'start bond must be positive'); | ||
| startingTime = block.timestamp + 3 days; |
There was a problem hiding this comment.
Why is the game start time 3 days after start is called?
There was a problem hiding this comment.
there's 3 days time to gather funds for both sides before the game actually starts. If we have this as 0, the game ends right away for the starters side as the other side did not have funds in the game the next block
| } | ||
|
|
||
| // TODO, verify that this is never bigger than nonDecisionThreshold and is always increasing or constant in terms of timeSinceStart | ||
| // approx for: attrition cost = start deposit * (nonDecisionThreshold / start deposit) ^ (time since start / time limit) |
There was a problem hiding this comment.
Consider renaming attrition cost throughout to escalation threshold. I don't think attrition is the right word here, and I found it confusing when trying to understand how the system works. Attrition usually means slow eating losses over time, but IIUC this represents a threshold one needs to reach in order to achieve some goal.
Another suggestion GLM 5 had was resolution floor.
There was a problem hiding this comment.
After back and forth with an AI, what made the system "click" for me is realizing that this is basically a race where people fill in pots of money and there is water filling the pots at an exponential rate. As soon as there is only one pot with money above water, that is the winner. If all of the pots overflow, then the game ends in a fork.
There was a problem hiding this comment.
yeah thats a good analogy. escalation threshold sounds like something fixed, its not fixed thought. Why the treshold would keep increasing? I feel attrition cost is more logical name
There was a problem hiding this comment.
I am pretty firmly of the belief that attrition is not the word you want. I'm not tightly coupled to escalation threshold either though. "High Water Mark" or "Low Water Mark" perhaps?
That being said, I don't see any problem with a threshold that moves. I don't think the term "threshold" implies immutability. It just means the point at which event occurs.
| uint8 noOver = balances[2] >= currentTotalCost ? 1 : 0; | ||
| if (invalidOver + yesOver + noOver >= 2) return YesNoMarkets.Outcome.None; // if two or more outcomes are over the total cost, the game is still going | ||
| // the game has ended due to timeout | ||
| // TODO, doesn't handle ties well logically. We could avoid it by checking if tie is happening before deposit and break it there |
There was a problem hiding this comment.
Also doesn't handle all 3 outcomes getting no deposits particularly well. Currently it falls back to No, but I feel like it should fall back to invalid in that case.
There was a problem hiding this comment.
the escalation game itself doesn't guarantee the starting condition, but if there's a proper start (managed by users of the escalation game), it should handle it okay?
There was a problem hiding this comment.
I think we should move the checks for starting bond into the escalation game, or the escalation game should have a reasonable ending if the start is never funded. I don't think we should depend on external constraints for this module to work properly, any constraints necessary for it to work should be checked within this module.
There was a problem hiding this comment.
I think we should move the checks for starting bond into the escalation game, or the escalation game should have a reasonable ending if the start is never funded. I don't think we should depend on external constraints for this module to work properly, any constraints necessary for it to work should be checked within this module.
I disagree with this, we don't want this to handle rep for example otherwise it becomes pain to move rep around. This game will have external dependencies that assume the user of the game uses it correctly and it wont work without external contract handling rep, forking and other management stuff
🛡️ Defender Against the Dark Arts✅ No issues found. Great job! |
🏛️ Architect2 issues found Tight coupling to concrete SecurityPool implementation📍 solidity/contracts/peripherals/EscalationGame.sol:176 EscalationGame directly calls Implicit coupling to caller type violates separation of concerns📍 solidity/contracts/peripherals/factories/EscalationGameFactory.sol:8 The deployEscalationGame function implicitly requires the caller (msg.sender) to be an ISecurityPool contract, casting it directly on line 9. This creates high coupling between the factory and its callers, breaking separation of concerns and making the system dependent on invocation context rather than explicit dependencies. This hinders testability, modularity, and violates principles like dependency inversion. |
🔧 Expensive Linter4 issues found Inconsistent script naming conventionThe new script key 'test-escalationGame' uses camelCase within the name, breaking the prevailing kebab-case convention used for all other multi-word scripts (e.g., 'test-peripherals', 'setup-contracts', 'compile-contracts'). npm script names should be consistent and typically follow kebab-case. Missing space before function body brace📍 solidity/contracts/peripherals/EscalationGame.sol:119 In the function definition on line 119, there is no space between the closing parenthesis of the parameter list and the opening brace, which is inconsistent with other functions in the file that have a space (e.g., lines 37, 48, 54). Constant naming convention violated📍 solidity/contracts/peripherals/EscalationGame.sol:15 The constant 'escalationTimeLength' on line 15 is defined in camelCase, but Solidity style conventions recommend using UPPER_SNAKE_CASE for constants to improve readability and consistency. Typographical errors in comment📍 solidity/contracts/peripherals/EscalationGame.sol:174 The comment on line 174 contains grammatical and spelling errors: 'against to' should be 'against the', 'treshold' should be 'threshold', and 'todo' should be capitalized to 'TODO' for consistency with other comments in the file. |
🔒 Security Reviewer1 issue found Missing Access Control on Game Deployment📍 solidity/contracts/peripherals/factories/EscalationGameFactory.sol:8 The deployEscalationGame function is marked external and lacks any access control mechanism, allowing any caller to deploy a new EscalationGame instance with an arbitrary security pool. This could lead to unauthorized deployments, potentially enabling attackers to deploy games with malicious security pools if the system intends to restrict game creation to trusted entities. |
🐛 Bug Hunter9 issues found Incorrect scaling in Taylor series for attrition cost📍 solidity/contracts/peripherals/EscalationGame.sol:58 The function compute5TermTaylorSeriesAttritionCostApproximation contains a scaling error in the third term of the Taylor series. The variable z is scaled by SCALE, but the term for y^5 uses z2z2z/(5*SCALE), which introduces an extra factor of SCALE, causing lnRatio to be overestimated and resulting in incorrect totalCost() and related time calculations. getMarketResolution does not handle ties correctly📍 solidity/contracts/peripherals/EscalationGame.sol:127 When two or more outcomes have exactly the same highest balance, the function returns YesNoMarkets.Outcome.No instead of indicating no decision (None). The strict inequality checks fail to detect ties, leading to an incorrect market outcome. withdrawDeposit can be called before market resolution📍 solidity/contracts/peripherals/EscalationGame.sol:201 The function only checks that nonDecisionTimestamp is zero, which does not guarantee the market has resolved. If called while the game is ongoing (getMarketResolution returns None), it passes None to claimDepositForWinning, likely causing a revert due to an empty deposit array. A require should ensure marketResolution != YesNoMarkets.Outcome.None. claimDepositForWinning uses flawed condition with double-counted deposit.amount📍 solidity/contracts/peripherals/EscalationGame.sol:181 The condition in the else-if branch, Constant salt in create2 deployment causes address collisions and reverts on duplicate deployments📍 solidity/contracts/peripherals/factories/EscalationGameFactory.sol:7 The deployEscalationGame function uses a fixed salt of bytes32(0) when deploying EscalationGame via create2. For identical inputs (same msg.sender, startBond, and nonDecisionThreshold), the deployed contract address will be identical. According to EIP-684, if the address already contains code, the creation will revert, causing the factory transaction to fail on subsequent deployments with the same parameters. This prevents multiple games from being deployed for the same security pool with identical thresholds and is an unintended behavior. Fix: Introduce a nonce state variable to ensure a unique salt per deployment. Increment the nonce with each call and incorporate it into the salt via keccak256. Pagination loop may skip deposits due to filter📍 solidity/ts/testsuite/simulator/utils/contracts/escalationGame.ts:38 The loop in getEscalationGameDeposits breaks when the filtered deposit list length is not equal to page size (30). This causes the pagination to stop prematurely if any deposits are filtered out (e.g., zero depositor), missing later deposits. The termination condition should be based on the raw number of entries returned by the contract before filtering. deployEscalationGame computes address with mismatched constructor arguments📍 solidity/ts/testsuite/simulator/utils/contracts/escalationGame.ts:63 The function calls the factory's deployEscalationGame with arguments [startBond, nonDecisionThreshold], but then computes the CREATE2 address using encodeDeployData with arguments [writeClient.account.address]. The computed address will not match the actual deployed contract because the init code hash differs. The args in encodeDeployData must match those used by the factory. deployEscalationGame incorrectly prefixes bytecode with 0x📍 solidity/ts/testsuite/simulator/utils/contracts/escalationGame.ts:62 The bytecode passed to encodeDeployData is constructed as deployEscalationGame uses non-32-byte salt for CREATE2📍 solidity/ts/testsuite/simulator/utils/contracts/escalationGame.ts:66 The salt passed to getCreate2Address is numberToBytes(0), which yields a 1-byte array. CREATE2 requires a 32-byte salt. Using a shorter salt produces an incorrect address. The salt should be padded to 32 bytes, e.g., numberToBytes(0, { size: 32 }). |
🧠 Wise Man8 issues found Inconsistent script naming convention in package.jsonThe new script 'test-escalationGame' uses camelCase with an uppercase letter, while all other multi-word npm scripts in this file follow kebab-case (e.g., 'test-peripherals', 'setup-contracts'). This breaks established naming consistency and violates the principle of predictable patterns. Naming should follow the existing convention of all lowercase with hyphens for multi-word identifiers. Typo in comment at line 174📍 solidity/contracts/peripherals/EscalationGame.sol:174 Comment contains misspelling: 'treshold' should be 'threshold' and 'games' should be 'game's'. Typo in comment at line 196📍 solidity/contracts/peripherals/EscalationGame.sol:196 Comment contains multiple typos: 'someones elses' should be 'someone else's'. Magic numbers in burn calculations📍 solidity/contracts/peripherals/EscalationGame.sol:186 The claimDepositForWinning function uses the literal numbers 2 and 5 in multiple places to calculate burn amounts. These should be replaced with named constants (e.g., EXCESS_PAYOUT_MULTIPLIER and BURN_DENOMINATOR) to improve readability and maintainability. Duplicated Taylor series computation📍 solidity/contracts/peripherals/EscalationGame.sol:64 The compute5TermTaylorSeriesAttritionCostApproximation function manually expands the 5-term series with repetitive code. This can be simplified using a loop to compute the series, reducing duplication and making it easier to adjust the number of terms. Duplicate outcome counting logic📍 solidity/contracts/peripherals/EscalationGame.sol:121 The functions getMarketResolution and hasReachedNonDecision both contain similar code to count how many outcomes exceed a threshold. This logic should be extracted into a helper function to adhere to the DRY principle and simplify maintenance. Unnecessary underscore prefix in function parameter name📍 solidity/contracts/peripherals/factories/EscalationGameFactory.sol:8 The parameter '_nonDecisionThreshold' uses an underscore prefix, which is unconventional for function parameters in Solidity. Typically, underscores are reserved for private state variables, not parameters. This can cause confusion and reduce readability by implying a different scope or purpose. Renaming to 'nonDecisionThreshold' aligns with common Solidity conventions and improves clarity. Inconsistent npm script naming conventionThe newly added npm script 'test-escalationGame' uses an uppercase letter 'G', while all other scripts in package.json adhere to lowercase kebab-case (e.g., 'test-peripherals', 'compile-contracts'). This inconsistency violates the universal best practice of consistent naming, which enhances readability and prevents potential invocation errors on case-sensitive systems. Rename the script to 'test-escalation-game' to align with the established convention. |
2 participants
No description provided.