security pool forker by KillariDev · Pull Request #53 · AugurProject/zoltar

KillariDev · 2026-02-13T12:51:10Z

removes forking code from security pool
create security pool forker contract that handles forking and auction
integrate to new zoltar
integrate to escalation game

…ion-game-and-new-zoltar

github-actions · 2026-02-23T16:33:04Z

🏛️ Architect

14 issues found

Breaking change in finalizeAuction function signature

📍 solidity/contracts/peripherals/Auction.sol:42

The modification adds a parameter 'receiver' to the public function finalizeAuction, altering its interface. This breaks backward compatibility for any existing external callers that do not provide this argument, causing transaction reverts and requiring mandatory updates in all dependent contracts or off-chain services. Such unversioned interface changes violate stable interface principles and can lead to integration failures in any deployment context.

Centralized State Manipulation via 'onlyForker' Functions

📍 solidity/contracts/peripherals/SecurityPool.sol:327

The SecurityPool contract introduces multiple functions (e.g., setSystemState, setRetentionRate, setVaultOwnership, etc.) that allow a privileged 'securityPoolForker' address to arbitrarily set critical state variables. This breaks encapsulation, risks state corruption if misused, and creates a backdoor that bypasses normal business logic and invariants. It is an anti-pattern that centralizes control without safeguards.

Mappings inside structs cause storage inefficiency and deletion limitations

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:16

The ForkData struct (lines 16-27) contains two mappings: 'children' (uint8 => ISecurityPool) and 'claimedAuctionProceeds' (address => bool). Storing mappings inside structs is an anti-pattern in Solidity because mappings cannot be deleted when the struct is deleted, leading to permanent storage allocation and increased gas costs for operations that interact with these mappings. The code comment on line 15 explicitly acknowledges this issue ('todo, move mappings outside the struct'). This violates Solidity best practices and negatively impacts long-term maintainability and storage efficiency.

Tight coupling to concrete Auction implementation

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:19

The SecurityPoolForker contract directly references the concrete Auction contract (imported from './Auction.sol') in the ForkData struct and in multiple function calls (startAuction, finalizeAuction, totalRepPurchased). This violates the Dependency Inversion Principle, which recommends depending on abstractions. The rest of the codebase consistently uses interfaces (ISecurityPool, IShareToken, ISecurityPoolForker), but for Auction the concrete class is used. This tight coupling reduces flexibility, complicates testing (cannot mock Auction), and makes future changes to the auction mechanism difficult without modifying SecurityPoolForker. A proper IAuction interface should be introduced and used instead.

Factory returns concrete implementation instead of interface, increasing coupling

📍 solidity/contracts/peripherals/factories/ShareTokenFactory.sol:13

The deployShareToken function's return type was changed from the IShareToken interface to the concrete ShareToken contract. This violates the dependency inversion principle by forcing clients to depend on a concrete implementation rather than an abstraction, reducing modularity, testability, and maintainability. The removal of the IShareToken import further solidifies this tight coupling, making the system less flexible for future changes or mock implementations.

IShareToken interface depends on concrete peripheral contract YesNoMarkets

📍 solidity/contracts/peripherals/interfaces/IShareToken.sol:5

IShareToken is a peripheral interface; it should depend on core abstractions, not on other peripheral implementations. The changes introduce a direct import of '../YesNoMarkets.sol' and replace all uses of Zoltar.Outcome with YesNoMarkets.Outcome. This creates a hard, concrete dependency on another peripheral contract, violating the Dependency Inversion Principle and typical layered architecture rules (outer layers should not depend on each other). It risks circular dependencies if YesNoMarkets also uses IShareToken, reduces reusability of the token abstraction across different market types, and increases coupling between peripheral modules.

TokenId library exhibits inappropriate coupling due to specific market dependency

📍 solidity/contracts/peripherals/tokens/TokenId.sol

The TokenId library is named generically, suggesting it should be reusable for various token ID schemes across the system. However, it now has a hard dependency on YesNoMarkets.Outcome, tying it specifically to yes/no market implementations. This violates architectural principles of separation of concerns and naming consistency, as a component in a general 'tokens' periphery directory should not be coupled to a specific market type. It hinders reusability for other token types (e.g., if scalar markets emerge) and introduces high coupling between the TokenId utility and a concrete market contract. This misalignment could lead to code duplication or forced refactoring if the system evolves to support multiple market types.

Overly Long Test Function Violates Single Responsibility

📍 solidity/ts/tests/testPeripherals.ts:255

The test 'two security pools with disagreement' (lines 255-425) spans 170+ lines and tests fork setup, migration, auctions, and redemption in one function. This violates test design principles: each test should verify one behavior. The length makes tests brittle, hard to debug, and difficult to maintain.

Monolithic simulation module with excessive responsibilities

📍 solidity/ts/testsuite/simulator/SimulationModeEthereumClientService.ts

The file SimulationModeEthereumClientService.ts is 729 lines long and exports over 30 functions covering a wide range of concerns: transaction simulation, block generation, balance/code queries, receipt generation, logs handling, fee history calculation, personal signing simulation, and more. This violates the Single Responsibility Principle and creates a God Module. Such a monolithic structure increases cognitive load, hinders maintainability and testing, and makes future extensions error-prone. The module should be decomposed into focused, cohesive units (e.g., transactionSimulation, blockSimulation, stateQuery, feeHistory, signing) to improve separation of concerns and long-term scalability.

God module anti-pattern: single file orchestrates entire peripheral deployment

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:64

The deploymentPeripherals module acts as a god object, containing intimate knowledge of all contract artifacts, their bytecode, ABIs, interdependencies, and deployment order. This violates separation of concerns, creates high coupling, and makes the system brittle. Any change to contract deployment requires modifying this central file. The module should be decomposed into focused components with clear responsibilities (e.g., address calculator, deployment executor, dependency resolver).

Parameter explosion in dependency passing indicates missing abstraction

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:27

Functions like getSecurityPoolFactoryByteCode and getSecurityPoolFactoryAddress accept 8 parameters representing contract dependencies. This leads to fragile APIs, difficulties in maintaining parameter order, and violation of Interface Segregation Principle. A better design would introduce a contract dependency registry or configuration object that encapsulates these relationships.

Mixed low-level bytecode manipulation with high-level deployment logic

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:13

The module contains low-level bytecode manipulation (applyLibraries, direct bytecode concatenation) alongside high-level deployment orchestration. This violates layering principles and mixes concerns. Bytecode manipulation should be isolated in a separate utility, while deployment should work with abstractions like DeployableContract.

Hardcoded deployment order and conditional logic

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:108

ensureInfraDeployed contains a hardcoded deployment sequence with conditional checks for each contract's existence. This approach is brittle and doesn't model explicit dependencies. A more robust solution would define a dependency graph and perform topological sorting to determine the deployment order automatically.

Missing chain configuration in createReadClient

📍 solidity/ts/testsuite/simulator/utils/viem.ts:9

The createReadClient function omits the 'chain' parameter when creating Viem clients. This parameter was present in the original implementation and is required for proper network configuration. Its absence can cause client initialization to fail or rely on implicit defaults, leading to unpredictable behavior. Furthermore, this omission creates an inconsistency with createWriteClient, which explicitly sets chain to mainnet, violating the principle of consistent abstraction across similar utilities.

github-actions · 2026-02-23T16:34:40Z

🛡️ Defender Against the Dark Arts

10 issues found

Explicit theft function named stealAllRep

📍 solidity/contracts/peripherals/SecurityPool.sol:372

The contract contains a function named 'stealAllRep' that allows the securityPoolForker to transfer all REP tokens held by the contract to themselves. This is an explicit backdoor with a name that suggests malicious intent. Even if intended as an emergency recovery, such naming is highly inappropriate and indicates potential compromise or severe security culture failure.

Unlimited token approval to Zoltar contract

📍 solidity/contracts/peripherals/SecurityPool.sol:99

The constructor calls repToken.approve(address(zoltar), type(uint256).max), granting Zoltar unlimited spending allowance over the contract's REP tokens. If Zoltar is compromised or contains a bug, it could drain all REP from the SecurityPool. This violates the principle of least privilege and creates a single point of failure.

Centralized administrative control with no safeguards

📍 solidity/contracts/peripherals/SecurityPool.sol:75

The contract defines an 'onlyForker' modifier that grants a single address sweeping powers: it can arbitrarily modify vault ownership, security bond allowances, global pool parameters, and directly steal assets via stealAllRep and migrateEth. There are no timelocks, multi-signature requirements, or rate limits. This is a critical centralization risk and a backdoor that could be exploited if the forker address is compromised or malicious.

Invalid SPDX License Identifier

📍 solidity/contracts/peripherals/factories/AuctionFactory.sol:1

The SPDX license identifier was incorrect ('UNICENSE') and has been corrected to 'Unlicense'. While this is a typographical fix, an invalid SPDX identifier can cause license compliance tools to misinterpret the licensing terms, potentially leading to legal risks or incorrect license scanning results. This is a high-confidence issue as it directly affects the repository's license metadata.

Incorrect ABI Encoding in Salt Calculation

📍 solidity/contracts/peripherals/factories/AuctionFactory.sol:7

The salt calculation changed from abi.encodePacked(msg.sender, salt) to abi.encode(msg.sender, salt). This alters the deterministic address generated by CREATE2 for the same inputs. While abi.encode is generally safer for hashing, the change may break existing integrations that rely on the previous salt computation, leading to unexpected contract deployments. This is a functional change with potential backward compatibility issues, but not a direct security vulnerability. However, given the factory pattern, unexpected address changes could cause misconfigurations in dependent systems.

Potential Backdoor via Centralized Control of SecurityPoolForker

📍 solidity/contracts/peripherals/factories/SecurityPoolFactory.sol:30

The new securityPoolForker parameter introduced in the constructor and the require statement in deployChildSecurityPool centralize control over pool deployment and auction ownership. The securityPoolForker address is set by the factory deployer and then becomes the sole entity allowed to deploy child pools and owns all resulting auctions. This creates a single point of failure and potential backdoor if the securityPoolForker is compromised or malicious.

Unclear Purpose of New Dependencies EscalationGameFactory and YesNoMarkets

📍 solidity/contracts/peripherals/factories/SecurityPoolFactory.sol:14

Three new contract dependencies are introduced (EscalationGameFactory, YesNoMarkets, ISecurityPoolForker) with no clear explanation of their purpose or trustworthiness in this context. These contracts now receive control over the SecurityPool construction and market creation, which could be vectors for supply chain compromise if these dependencies are not properly vetted.

Changed Ownership Model May Bypass SecurityPool Control

📍 solidity/contracts/peripherals/factories/SecurityPoolFactory.sol:54

The truthAuction ownership is now set to securityPoolForker (line 54) instead of the newly deployed securityPool (as in original). This gives an external entity (securityPoolForker) direct control over the auction rather than the pool itself, potentially bypassing pool governance and creating an unauthorized administrative channel.

Suspicious 'stealAllRep' function added to ISecurityPool interface

📍 solidity/contracts/peripherals/interfaces/ISecurityPool.sol:95

The function 'stealAllRep()' is introduced in the interface, implying the ability to steal all reputation tokens from the security pool. The explicit naming suggests malicious intent or a backdoor, as legitimate administrative functions should not be termed 'steal'. If implemented without strict access control, this could allow an attacker to drain all REP tokens, posing a critical risk to the system's assets.

Hardcoded suspicious network endpoint

📍 solidity/ts/testsuite/simulator/utils/viem.ts:6

The code defines a constant DEFAULT_HTTP with value 'https://ethereum.dark.florist', which is used as the transport URL when no injected provider is present. This domain is not a known legitimate Ethereum node and the name suggests malicious intent ('dark'). Hardcoding such an endpoint could route Ethereum traffic to an attacker-controlled server, enabling data exfiltration, transaction manipulation, or phishing attacks. Additionally, the removal of explicit 'chain: mainnet' configuration (in the diff) means the client may accept whatever chain the malicious node presents, increasing risk of chain confusion attacks. This pattern is highly indicative of a backdoor or supply chain compromise.

github-actions · 2026-02-23T16:37:17Z

🔧 Expensive Linter

22 issues found

Inconsistent currency terminology in error messages

📍 solidity/contracts/peripherals/Auction.sol:48

The error message in finalizeAuction uses 'Ether' while other parts of the code (e.g., 'need to invest with eth!' and variable names like 'ethAmountToBuy') use 'eth' for the same currency, leading to inconsistency.

Inconsistent capitalization in revert reason strings

📍 solidity/contracts/peripherals/SecurityPool.sol:70

Revert reason strings in require statements exhibit inconsistent capitalization. Most revert reasons start with a lowercase letter (e.g., 'only callable by securityPoolFactory', 'failed to send Ether', 'need to send eth'), but several start with an uppercase letter (e.g., 'OnlyOracle', 'Stale price', 'Only Forker', 'Market has not finalized!', 'Not enough REP'). This reduces code consistency and readability. All revert reasons should follow a uniform style, preferably with the first word in lowercase unless it's a proper noun (e.g., token symbols like 'REP' may remain uppercase).

Incorrect SPDX license identifier

📍 solidity/contracts/peripherals/SecurityPoolUtils.sol:1

The SPDX license identifier 'UNICENSE' is a typo and not a valid SPDX identifier. The correct identifier for the Unlicense is 'Unlicense'. This change corrects the identifier to the standard form, ensuring compliance and avoiding potential legal ambiguity.

Typographical error in comment

📍 solidity/contracts/peripherals/SecurityPoolUtils.sol:29

The comment contains a misspelling: 'decreases' should be 'decreases'. This correction improves readability and maintains professional documentation standards.

String literals must use double quotes

📍 solidity/contracts/peripherals/YesNoMarkets.sol:22

In Solidity, single quotes are only valid for single-byte characters (e.g., bytes1). Multi-character string literals must be enclosed in double quotes. The code uses single quotes for string literals in three places, causing a compilation error.

Missing separation of view and mutative functions in ISecurityPool interface

📍 solidity/contracts/peripherals/interfaces/ISecurityPool.sol:79

The original ISecurityPool interface clearly separated view and mutative functions with a '// -------- Mutative Functions --------' comment. The updated version removed this comment and placed view functions (escalationGame, feeIndex, yesNoMarkets, securityPoolForker) after mutative functions, breaking the established convention. This hampers readability and maintainability.

Inconsistent parameter naming in ISecurityPool setter functions

📍 solidity/contracts/peripherals/interfaces/ISecurityPool.sol:80

The new setter functions in ISecurityPool use three different naming conventions for parameters: plain descriptive names (e.g., poolOwnership), 'new' prefix (e.g., newRetention), and leading underscores (e.g., _poolOwnership). The existing codebase (e.g., setStartingParams) uses plain names without prefixes, indicating a convention of direct camelCase. This inconsistency should be standardized to a single convention.

Missing NatSpec documentation block for interface

📍 solidity/contracts/peripherals/interfaces/IShareToken.sol:6

The interface IShareToken is missing the required NatSpec comment with @title and @notice that was present in the original file. This documentation is important for generating documentation and clarifying the interface's purpose.

Incorrect SPDX license identifier capitalization

📍 solidity/contracts/peripherals/interfaces/IWeth9.sol:1

The SPDX license identifier 'UNICENSE' is incorrectly capitalized. The official SPDX identifier for the Unlicense is 'Unlicense' (capital 'U', rest lowercase). Using an invalid identifier may cause tooling to fail to recognize the license and could lead to licensing ambiguities.

Grammatically incorrect and unclear TODO comment

📍 solidity/contracts/peripherals/openOracle/OpenOracle.sol:497

The comment on line 497 contains grammatical errors ('it's bit hard' should be 'it's a bit hard' or similar) and uses unclear phrasing ('commented as' is misleading since the code is not commented out). This reduces code readability and maintainability, and is inconsistent with the professional tone of other comments in the codebase.

Invalid string literal: single quotes used

📍 solidity/contracts/peripherals/tokens/ForkedERC1155.sol:21

The require statement uses single quotes for a string literal, which is invalid in Solidity. String literals must be enclosed in double quotes. This will cause a compilation error.

Unused import

📍 solidity/contracts/peripherals/tokens/ForkedERC1155.sol:5

The file imports '../../Constants.sol' but does not use any symbols from it. This import should be removed to avoid unnecessary dependencies and keep the code clean.

Inconsistent require condition style in new function

📍 solidity/contracts/peripherals/tokens/ShareToken.sol:79

The newly added burnTokenId function uses a redundant boolean comparison in the require statement (require(authorized[msg.sender] == true, ...)), while the existing authorize function uses the boolean directly (require(authorized[msg.sender], ...)). For consistency, all authorization checks should use the boolean without comparison to improve readability and adhere to common Solidity style guides.

Typographical error in comment

📍 solidity/ts/testsuite/simulator/SimulationModeEthereumClientService.ts:18

Comment on line 18 contains a misspelled word 'transatons' which should be 'transactions'.

Typographical error in error message

📍 solidity/ts/testsuite/simulator/SimulationModeEthereumClientService.ts:214

Error message on line 214 has a misspelled word 'transction' which should be 'transaction'.

Inconsistent variable casing in getMarketId function

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:143

The variable 'securityPoolfactory' uses lowercase 'f' in 'factory', deviating from camelCase convention where each word starts with uppercase (e.g., 'securityPoolUtils'). It should be 'securityPoolFactory' for consistency with other variables and the property 'securityPoolFactory' from getInfraContractAddresses().

Inconsistent trailing commas in multiline object literals

📍 solidity/ts/testsuite/simulator/utils/contracts/peripherals.ts:90

The file uses two different styles for trailing commas in multiline object literals. Objects in client.writeContract calls include trailing commas on every property, while return objects in getOpenOracleExtraData and getOpenOracleReportMeta do not. Additionally, getOpenOracleExtraData itself mixes styles (lines 92–93 have commas, lines 90–91 and 94–98 do not). This violates consistency; the file should adopt a single convention.

Inconsistent array destructuring formatting

📍 solidity/ts/testsuite/simulator/utils/contracts/securityPoolForker.ts:114

In line 114, the array destructuring has a space after the opening bracket ('['), which is inconsistent with other array literals in the file that have no space after '[' or before ']'. This deviates from common TypeScript/JavaScript formatting conventions.

Inconsistent conversion of QuestionOutcome parameters

📍 solidity/ts/testsuite/simulator/utils/contracts/securityPoolForker.ts:123

In migrateVault and createChildUniverse, QuestionOutcome parameters are explicitly converted to number using Number(), but in migrateFromEscalationGame, the outcomeIndex parameter (of the same type) is passed directly without conversion. This inconsistency may cause type uncertainty or reduce code clarity, especially when interfacing with contract calls expecting numeric values.

Unintended spaces in template literal interpolations

📍 solidity/ts/testsuite/simulator/utils/testUtils.ts:13

In the approximatelyEqual function's default error message (line 13), the template literals contain spaces inside the interpolation braces (e.g., ${ errorDelta } and ${ diff }). This results in extra spaces in the generated error message string, leading to poorly formatted output such as 'within 5n' instead of 'within 5n'. The spaces should be removed for correct interpolation.

Inconsistent function declaration style for top-level exports

📍 solidity/ts/testsuite/simulator/utils/typescript.ts:38

The newly added objectEntries function is declared using an arrow function assigned to a const variable, while all other top-level exported functions in this file use the function keyword declaration. This breaks consistency with the established pattern in the codebase.

Import order violates external-before-internal grouping

📍 solidity/ts/testsuite/simulator/utils/viem.ts:3

The import statement for the internal module './bigint.js' (line 3) appears before the import for the external module 'viem/chains' (line 4). The original codebase placed external imports before internal ones, and this change deviates from that likely convention of grouping external dependencies before local modules.

github-actions · 2026-02-23T16:39:07Z

🧠 Wise Man

35 issues found

Commented-out code in finalizeAuction

📍 solidity/contracts/peripherals/Auction.sol:43

The function finalizeAuction contains a commented-out require statement that checks auction timing. Commented-out code adds visual noise, can mislead readers, and should be removed to maintain code cleanliness. Rely on version control for historical context instead.

Unused event FinalizedAuction

📍 solidity/contracts/peripherals/Auction.sol:15

The event FinalizedAuction is defined but never emitted in the contract. Unused definitions increase cognitive load and suggest incomplete implementation. It should be either removed or integrated by emitting it within finalizeAuction with appropriate parameters.

Simplify getBindingCapital using min/max pattern

📍 solidity/contracts/peripherals/EscalationGame.sol:141

The current implementation uses nested conditionals to find the median of three balances. This can be replaced with a simpler and more readable expression: sum of all three minus min and max. This improves clarity and reduces cognitive load.

Spelling error in comment

📍 solidity/contracts/peripherals/EscalationGame.sol:174

The word 'treshold' in the comment for claimDepositForWinning is misspelled. It should be 'threshold'. Correcting such typographical errors improves code professionalism and readability.

Incorrect ETH refund logic in requestPriceIfNeededAndQueueOperation

📍 solidity/contracts/peripherals/PriceOracleManagerAndOperatorQueuer.sol:113

The function refunds the entire contract balance to the sender, rather than only the excess ETH sent by the caller for this transaction. This can lead to unauthorized withdrawals if the contract holds funds from other sources, as the refund amount should be limited to msg.value minus any costs incurred (i.e., ethCost when requestPrice is called).

Global security bond allowance inconsistency in forker setter functions

📍 solidity/contracts/peripherals/SecurityPool.sol:335

The state variable totalSecurityBondAllowance is intended to be the sum of all individual vault securityBondAllowance values. However, the functions setVaultSecurityBondAllowance and setVaultOwnership modify vault allowances without updating totalSecurityBondAllowance, causing a discrepancy. This affects fee calculations in updateCollateralAmount and security checks in createCompleteSet and performSetSecurityBondsAllowance.

Locked REP not decreased in withdrawFromEscalationGame

📍 solidity/contracts/peripherals/SecurityPool.sol:315

In withdrawFromEscalationGame, when rep is withdrawn from the escalation game and added to the vault's pool ownership, the lockedRepInEscalationGame counter is not decreased. This causes lockedRepInEscalationGame to overstate the actual locked amount, leading to incorrect rep redemption in redeemRep where it is subtracted from the pool rep amount.

Incorrect repAmount calculation in redeemRep due to lockedRep subtraction

📍 solidity/contracts/peripherals/SecurityPool.sol:292

In redeemRep, the repAmount is calculated as poolOwnershipToRep(securityVaults[vault].poolOwnership) minus securityVaults[vault].lockedRepInEscalationGame. However, lockedRepInEscalationGame represents rep deposited in the escalation game, which is not part of the pool's rep balance. Subtracting it results in under-redeeming rep for vault owners. The function should redeem the full pool rep amount without subtracting locked rep.

Mappings defined inside struct should be moved to contract level

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:16

The ForkData struct contains mappings 'children' and 'claimedAuctionProceeds'. In Solidity, mappings inside structs are an anti-pattern due to gas inefficiency and complexity. The code includes a TODO comment (line 15) acknowledging this issue. These mappings should be defined as separate contract-level mappings, and the struct should only contain value types for better maintainability and gas optimization.

Hard-coded outcome count in forkSecurityPool

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:73

The array outcomeIndices is created with a hard-coded size of 5 (from '4 + 1'), assuming exactly 5 outcomes. This magic number reduces readability and maintainability; it should be replaced with a named constant or derived from configuration to avoid assumptions and ease future changes.

Invalid SPDX license identifier

📍 solidity/contracts/peripherals/SecurityPoolUtils.sol:1

The SPDX license identifier 'UNICENSE' is invalid and non-standard. The correct identifier for the Unlicense is 'Unlicense' (capitalized). Using an invalid identifier can break license compliance tooling and cause ambiguity in licensing.

Unused enum 'Outcome'

📍 solidity/contracts/peripherals/YesNoMarkets.sol:5

The Outcome enum (lines 5-10) is defined but never referenced anywhere in the contract. Dead code increases cognitive load, causes confusion about intended functionality, and may reflect incomplete refactoring. It should be removed unless reserved for future external use.

Unused imports should be removed

📍 solidity/contracts/peripherals/factories/PriceOracleManagerAndOperatorQueuerFactory.sol:3

The file imports ShareToken, ISecurityPool, and Zoltar but none of these symbols are referenced within the contract. Unused imports clutter the codebase, increase compilation time, and may cause confusion or errors if the imported files change. Removing them improves maintainability and readability.

Event signature change in DeploySecurityPool breaks backward compatibility

📍 solidity/contracts/peripherals/factories/SecurityPoolFactory.sol:28

The event DeploySecurityPool has changed its parameter types from (uint192 universeId, uint56 questionId) to (uint248 universeId, uint256 marketId). This changes the event's topic hash, breaking existing off-chain subscribers that rely on the old signature.

Function signature change in deployChildSecurityPool breaks existing callers

📍 solidity/contracts/peripherals/factories/SecurityPoolFactory.sol:41

The function deployChildSecurityPool has changed its parameter list and return type. Previously, it took (IShareToken, uint192, uint56, uint256, uint256, uint256, uint256) and returned ISecurityPool. Now it takes (ISecurityPool, IShareToken, uint248, uint256, uint256, uint256, uint256, uint256) and returns (ISecurityPool, Auction). This changes the function selector and return ABI, breaking any existing callers.

Function signature change in deployOriginSecurityPool breaks existing callers

📍 solidity/contracts/peripherals/factories/SecurityPoolFactory.sol:58

The function deployOriginSecurityPool has changed its parameter list. Previously, it took (uint192, uint56, uint256, uint256, uint256, uint256) and returned ISecurityPool. Now it takes (uint248, string, uint256, uint256, uint256, uint256) and returns ISecurityPool. This changes the function selector, breaking any existing callers.

SPDX license identifier typo

📍 solidity/contracts/peripherals/interfaces/IWeth9.sol:1

The SPDX license identifier 'UNICENSE' is misspelled. The correct identifier for the Unlicense is 'Unlicense' (capital 'U', lowercase rest). This typo could cause issues with license recognition tools or compliance scanning.

Redundant public mapping and accessor for total supply

📍 solidity/contracts/peripherals/tokens/ERC1155.sol:19

The _supplies mapping is declared public, which automatically generates a getter function _supplies(uint256). Additionally, a separate totalSupply(uint256) function is implemented that returns the same value. This redundancy violates the DRY principle, introduces unnecessary surface area, and can cause confusion about the intended interface. It is better to make _supplies private and rely solely on the totalSupply function as the public accessor, especially if it is required by the IERC1155 interface.

Missing uniqueness validation for outcomes array

📍 solidity/contracts/peripherals/tokens/ForkedERC1155.sol:27

The migrate function does not validate that the outcomes array contains unique values. This allows a user to migrate the same balance multiple times to the same child universe by specifying duplicate outcome indices, resulting in token inflation as the total supply increases incorrectly for each duplicate.

Loop counter overflow due to uint8 type

📍 solidity/contracts/peripherals/tokens/ForkedERC1155.sol:29

The for-loop in migrate uses a uint8 counter i, which overflows when outcomes.length exceeds 255. This causes the loop to wrap around and potentially run indefinitely, leading to out-of-gas reverts or incorrect processing of outcomes.

Unused import

📍 solidity/contracts/peripherals/tokens/TokenId.sol:4

The file imports '../../Zoltar.sol' but does not use any of its symbols after the refactoring. This import should be removed to avoid unnecessary dependencies and compile-time overhead.

Duplicate function signature in ERC1155 ABI

📍 solidity/ts/abi/abis.ts:4

The ERC1155 ABI array defines two functions, '_supplies' and 'totalSupply', with identical signatures (single uint256 parameter, returns uint256). In Solidity, function selectors are based on name and parameter types; identical signatures cause interface conflicts and would prevent the ABI from correctly describing a valid contract. The '_supplies' function appears redundant and conflicts with the standard 'totalSupply' function, indicating a data integrity issue in the ABI definition.

Inconsistent variable naming: 'priorRepbalance' should be 'priorRepBalance'

📍 solidity/ts/tests/test.ts:51

The variable name 'priorRepbalance' does not follow the camelCase convention used throughout the file (e.g., 'afterForkBalance', 'priorSplitBalance'). Consistency in naming improves readability and reduces cognitive load. Rename the variable to 'priorRepBalance'.

Magic number used instead of constant for genesis universe identifier

📍 solidity/ts/tests/test.ts:29

In the test 'canDeployContract', the literal 0n is used to denote the genesis universe (line 29), even though a constant genesisUniverse is defined at line 15. This duplication violates the DRY principle and could lead to inconsistencies if the value ever changes. Use the genesisUniverse constant for clarity and maintainability.

Incorrect import syntax for node:test

📍 solidity/ts/tests/testPeripherals.ts:1

The import statement is written as import test, { describe, beforeEach } from 'node:test'. The node:test module only provides named exports (test, describe, beforeEach, etc.) and does not have a default export. This syntax imports the entire module namespace object into the variable test, which is not a function. When the test tries to call test('...'), it will throw a TypeError: test is not a function, breaking the entire test suite. The correct import should be import { test, describe, beforeEach } from 'node:test' to import the named test function directly.

Redundant infrastructure address computation in ensureInfraDeployed

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:103

The function ensureInfraDeployed calls getInfraContractAddresses() to obtain contract addresses, then calls getInfraDeployedInformation() which internally calls getInfraContractAddresses() again. Since getInfraContractAddresses() is a pure function that always returns the same deterministic addresses for a given deployment context, the second call is unnecessary and wastes computational resources. This pattern could also become problematic if the address computation ever gains side effects or becomes expensive.

Instead, reuse the already computed contractAddresses from the initial call to perform the existence checks directly, eliminating the redundant computation and making the control flow clearer.

Incorrect relative import path after file move

📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:6

The import for 'deployPeripherals.js' uses a relative path (./deployPeripherals.js) that assumes the file is in the same directory. After moving deployments.ts into the 'contracts' subdirectory, the 'deployPeripherals.js' file likely remains in the parent 'utils' directory. All other imports from the parent directory were updated to use '../', but this one was missed, causing a module resolution error at runtime. It should be changed to '../deployPeripherals.js'.

Dead code: commented-out old implementation of getUniverseName

📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:14

The function getUniverseName contains a block comment with the previous complex implementation. This dead code is no longer used and only adds noise, making the code harder to read and maintain. It should be removed entirely.

Unsafe conversion of bigint to number in migrateShares

📍 solidity/ts/testsuite/simulator/utils/contracts/peripherals.ts:266

The migrateShares function converts outcomes (bigint[]) to number[] via Number(x). Token IDs can exceed Number.MAX_SAFE_INTEGER (2^53-1), causing precision loss and incorrect migration. The contract expects uint256[], which viem can handle directly as bigint[] without conversion.

Inconsistent and missing return type annotations

📍 solidity/ts/testsuite/simulator/utils/contracts/securityPool.ts:6

The majority of functions lack explicit return type annotations, while getSystemState provides one. This inconsistency reduces type safety and code clarity. Additionally, some read functions use manual type assertions (e.g., as bigint) which may be unnecessary if the ABI is properly typed. All functions should have explicit return types aligned with their contract call return types, and manual casts should be removed where possible to rely on ABI-derived typing.

Inconsistent address comparison methods

📍 solidity/ts/testsuite/simulator/utils/logExplaining.ts:32

The code uses two different strategies for comparing Ethereum addresses: numeric comparison via BigInt (line 32) and case-insensitive string comparison (line 75). This inconsistency may cause address matching to fail when addresses have different zero padding, as numeric comparison normalizes padding while string comparison does not. To ensure reliability and maintainability, a single, consistent method should be used throughout the codebase. The numeric comparison is more robust because it treats equivalent addresses correctly regardless of leading zeros or case. Consider extracting a shared utility function like addressesEqual to centralize the logic and prevent future regressions.

objectEntries return type incorrectly omits numeric index property types

📍 solidity/ts/testsuite/simulator/utils/typescript.ts:38

The return type uses T[keyof T & string], which excludes properties indexed by number (numeric index signatures). For objects with only numeric index signatures (e.g., { [i: number]: boolean }), the value type boolean is not part of T[keyof T & string], causing the cast to be invalid and leading to type errors in consuming code. The return type should be [string, T[keyof T]][] to include all possible property value types, aligning with the actual values returned by Object.entries.

Invalid regular expression syntax in jsonParse

📍 solidity/ts/testsuite/simulator/utils/utilities.ts:29

The regular expression in jsonParse uses an invalid group syntax '(:', which causes a syntax error. It should be '(?' to create a named capture group. This breaks JSON parsing for byte strings.

Magic strings in ensureProxyDeployerDeployed

📍 solidity/ts/testsuite/simulator/utils/utilities.ts:212

The ensureProxyDeployerDeployed function contains several hardcoded strings (expected bytecode, funder address, funding amount, raw transaction). These magic values obscure intent and should be extracted into module-level constants (e.g., EXPECTED_PROXY_DEPLOYER_BYTECODE, PROXY_DEPLOYER_FUNDER_ADDRESS, DEPLOYER_FUNDING_AMOUNT, PROXY_DEPLOYER_RAW_TX).

Inconsistent chain configuration in createReadClient

📍 solidity/ts/testsuite/simulator/utils/viem.ts:9

The createReadClient function omits the chain parameter when creating PublicClient and WalletClient instances, while createWriteClient explicitly sets chain: mainnet. This inconsistency creates ambiguity about which network is used for read operations and may lead to unexpected behavior if the default chain differs from mainnet. For maintainability and clarity, both factory functions should explicitly specify the chain.

github-actions · 2026-02-23T16:39:47Z

🔒 Security Reviewer

17 issues found

Missing require(repToBuy > 0) in participate

📍 solidity/contracts/peripherals/Auction.sol:26

The participate function does not validate that repToBuy is positive. A participant can send ETH (msg.value > 0) while setting repToBuy to 0, resulting in loss of their ETH without receiving any REP.

Missing price validation in auction participation

📍 solidity/contracts/peripherals/Auction.sol:28

The participate function does not ensure that the ETH sent (msg.value) is proportional to the amount of REP purchased (repToBuy) based on the auction's total ETH (ethAmountToBuy) and total REP (repAvailable). This allows participants to acquire REP below the intended price, causing financial loss to the auction seller.

Incorrect payout calculation in claimDepositForWinning leading to potential theft

📍 solidity/contracts/peripherals/EscalationGame.sol:174

The claimDepositForWinning function contains a logic error in the second condition where it uses (deposit.cumulativeAmount + deposit.amount) to compute the excess over maxWithdrawableBalance. Since deposit.cumulativeAmount already includes deposit.amount, this effectively double-counts the deposit, resulting in an inflated excess and consequently an excessive amountToWithdraw. This can allow a depositor to receive more than their deposited funds and potentially exceed the total available pool, causing a loss of funds. Additionally, the use of getBindingCapital() (median of balances) as the cap is inconsistent with the intended use of nonDecisionThreshold as noted in the TODO comment.

Example: With balances: Winning outcome total=200, losing balances L1=100, L2=50, maxWithdrawableBalance=100. For a deposit of 150 where cumulative=200, the function returns 150 (case1). For a deposit of 150 where cumulative=90 (impossible since cum includes deposit, but if deposit=150, cum>=150), but regardless, the flawed case2 can produce amounts > deposit.

The correct approach should use nonDecisionThreshold as the cap and compute the amount based on the depositor's proportional share of the winning balance, ensuring total withdrawals do not exceed the pool.

Reentrancy and incorrect ETH refund in requestPriceIfNeededAndQueueOperation

📍 solidity/contracts/peripherals/PriceOracleManagerAndOperatorQueuer.sol:128

The function refunds the entire contract balance to the caller after state changes, allowing reentrancy attacks that could drain contract funds. Additionally, it refunds more than the caller's payment, enabling theft of funds held in the contract from other sources, such as excess ETH from requestPrice() calls.

requestPrice called without forwarding ETH in requestPriceIfNeededAndQueueOperation

📍 solidity/contracts/peripherals/PriceOracleManagerAndOperatorQueuer.sol:125

When a new price request is needed, the function calls requestPrice() internally without forwarding any ETH, causing requestPrice() to revert due to insufficient msg.value. This breaks the functionality of requesting prices when needed and may prevent price updates, leading to stale prices and potential system manipulation.

Missing access control on forkSecurityPool

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:60

The function forkSecurityPool is public and does not verify the caller's identity. Any attacker can fork any operational security pool that has a forked universe, leading to theft of the pool's REP tokens and disruption of the system.

Missing access control on forkZoltarWithOwnEscalationGame

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:208

The function forkZoltarWithOwnEscalationGame is public and lacks authorization checks. Attackers can call it on any security pool whose escalation game has triggered (nonDecisionTimestamp > 0), stealing the pool's REP and forking the universe.

Vault ownership lost when migrating before truth auction due to child REP balance check

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:144

In migrateVault, the condition child.repToken().balanceOf(address(child)) != 0 prevents setting the vault's pool ownership if the child security pool has no REP balance. Since the child's REP balance is typically zero before the truth auction, any vault migrated during this period will lose its REP share entirely (parent ownership is set to zero but child ownership is never set).

Division by zero risk in finalizeTruthAuction due to insufficient haircut in startTruthAuction

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:179

When calculating the amount of REP to sell in startTruthAuction, the code subtracts migratedRep / MAX_AUCTION_VAULT_HAIRCUT_DIVISOR from repAtFork. If migratedRep is less than the divisor, this subtraction yields repAtFork, meaning all remaining REP is offered for sale. If the auction then sells all offered REP, repPurchased equals repAvailable, causing a division by zero in _finalizeTruthAuction and permanently bricking the contract.

Incorrect outcomes array population in getForkingData

📍 solidity/contracts/peripherals/YesNoMarkets.sol:35

The getForkingData function returns a string array of size 4 for outcomes but only assigns values to indices 0 and 1 with 'Yes' and 'No', leaving indices 2 and 3 uninitialized (empty strings). Moreover, the assigned strings do not correspond to the Outcome enum values (0: Invalid, 1: Yes, 2: No, 3: None). This bug can lead to incorrect data being used for market resolution or forking logic, potentially compromising system integrity through incorrect payouts or state corruption.

Unsafe abi.encodePacked Usage for Salt Generation

📍 solidity/contracts/peripherals/factories/PriceOracleManagerAndOperatorQueuerFactory.sol:12

The original code used abi.encodePacked(msg.sender, salt) to generate a salt for create2. abi.encodePacked concatenates arguments without padding, which can lead to hash collisions when hashing inputs of different types. Specifically, an attacker could craft different (msg.sender, salt) pairs that produce identical packed bytes (e.g., (address(0x123), bytes32(0x456...)) vs (bytes32(0x123...), bytes32(0x456...)) if types are mismatched). This could allow multiple deployments to the same address, breaking expected uniqueness. The fix uses abi.encode which includes type padding, eliminating this risk.

Missing critical constructor parameter for ShareToken deployment

📍 solidity/contracts/peripherals/factories/ShareTokenFactory.sol:13

The ShareTokenFactory.deployShareToken() function no longer passes the required questionId parameter to the ShareToken constructor. The ShareToken contract likely requires this parameter to function correctly (e.g., to link to a specific prediction market question). Deploying tokens without proper initialization will result in non-functional or misconfigured tokens, potentially breaking protocol functionality and creating economic vulnerabilities.

Missing ERC1155 contract safety checks in transfer functions

📍 solidity/contracts/peripherals/tokens/ERC1155.sol:110

The safeTransferFrom and safeBatchTransferFrom functions do not implement the required ERC1155 safety mechanism to verify contract recipients. Transfers to contracts must call onERC1155Received or onERC1155BatchReceived and validate the return value to prevent tokens from being sent to incompatible contracts and lost permanently. This violates the ERC1155 standard and poses a critical risk of token loss.

Insufficient input validation in migrate function

📍 solidity/contracts/peripherals/tokens/ForkedERC1155.sol:19

The migrate function lacks essential input validation on the 'outcomes' array. Specifically: (1) It does not ensure the array is non-empty, causing tokens to be burned without migration if empty. (2) It does not check for duplicate outcome indices, allowing the same child token to receive the balance multiple times, inflating total supply. (3) It does not verify that outcome indices are within the valid range (1 to Constants.NUM_OUTCOMES), potentially migrating tokens to non-existent or unintended universes, leading to loss of funds. These flaws can be exploited by attackers to cause token loss or inflation.

mintCompleteSets function is payable but does not use msg.value

📍 solidity/contracts/peripherals/tokens/ShareToken.sol:52

The function mintCompleteSets is declared as payable but does not utilize the msg.value parameter. This allows callers to send ether to the contract, which will be permanently locked, resulting in loss of funds.

Precision Loss in migrateShares Conversion

📍 solidity/ts/testsuite/simulator/utils/contracts/peripherals.ts:266

The migrateShares function converts bigint[] outcomes to number[] using Number(x) on line 266. JavaScript numbers are IEEE 754 doubles with exact integer precision only up to 2^53-1. For Solidity uint256 values exceeding this, precision is lost, causing incorrect migration outcomes and potential fund loss.

Loss of precision from bigint to number conversion for uint256 contract parameters

📍 solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts:74

In the splitRep, forkerClaimRep, and deployChild functions, bigint outcome indexes are converted to JavaScript Numbers before being sent to the contract. Since JavaScript Numbers cannot accurately represent integers larger than 2^53-1, this can cause incorrect values to be sent to the contract, potentially leading to incorrect state changes, loss of reputation tokens, or execution on unintended outcomes. The contract expects uint256 values, which should be passed as bigints without conversion.

github-actions · 2026-02-23T16:45:01Z

🐛 Bug Hunter

49 issues found

User rep balance overwritten in participate

📍 solidity/contracts/peripherals/Auction.sol:29

In the participate function, purchasedRep[msg.sender] is assigned repToBuy, which overwrites any previous purchase instead of accumulating. This causes the mapping to store only the last purchase amount per user, not the total rep purchased, leading to incorrect user balance tracking.

Missing price validation in participate

📍 solidity/contracts/peripherals/Auction.sol:28

The participate function accepts any repToBuy and msg.value without ensuring the ETH sent is proportional to the rep bought. This allows users to underpay or overpay arbitrarily, breaking the auction's economic model where rep should be sold at a consistent price based on ethAmountToBuy and repAvailable.

Incorrect tie-breaking in getMarketResolution

📍 solidity/contracts/peripherals/EscalationGame.sol:119

The function getMarketResolution returns YesNoMarkets.Outcome.No by default when there is a tie for the highest balance, which can produce an incorrect outcome. For example, if Invalid and Yes have the same highest balance and No is lower, the function returns No instead of indicating a tie or using a proper tie-breaker.

Balances not decremented after deposit claims

📍 solidity/contracts/peripherals/EscalationGame.sol:175

In claimDepositForWinning, when a deposit is claimed, the corresponding balance in balances[uint8(outcome)] is not reduced by the deposited amount. This causes balances to remain inflated, leading to incorrect binding capital calculations, potential over-withdrawal in subsequent claims, and state inconsistency.

Missing input validation in claimDepositForWinning and withdrawDeposit

📍 solidity/contracts/peripherals/EscalationGame.sol:175

claimDepositForWinning does not validate that depositIndex is within bounds or that outcome is a valid market outcome (not Outcome.None). This can cause out-of-bounds array access or incorrect deposit claims. Additionally, withdrawDeposit may call claimDepositForWinning with outcome from getMarketResolution(), which can be Outcome.None if the game is still ongoing, without checking.

Incorrect ETH refund logic in requestPriceIfNeededAndQueueOperation

📍 solidity/contracts/peripherals/PriceOracleManagerAndOperatorQueuer.sol:122

The function refunds the entire contract balance to the sender after processing, which may include ETH from previous transactions, instead of refunding only the excess from the current transaction minus any price request cost. This can lead to loss of funds for previous depositors. The refund should be based on msg.value and only deduct ethCost if a price request was made in this call.

depositRep missing liquidation check

📍 solidity/contracts/peripherals/SecurityPool.sol:190

The depositRep function no longer verifies whether a liquidation operation is pending for the vault via PriceOracleManagerAndOperatorQueuer. This allows vault owners to deposit rep to avoid imminent liquidation, breaking the intended liquidation protection.

redeemShares incorrect accounting

📍 solidity/contracts/peripherals/SecurityPool.sol:282

redeemShares burns outcome tokens and sends ETH without reducing completeSetCollateralAmount and shareTokenSupply, and does not call updateCollateralAmount before calculating the payout. This results in overpayment and corrupts the pool's NAV, leading to fund loss.

redeemRep missing denominator adjustment

📍 solidity/contracts/peripherals/SecurityPool.sol:293

redeemRep zeros a vault's poolOwnership but does not reduce poolOwnershipDenominator. This breaks the invariant that total ownership equals denominator, causing misallocation of rep among remaining vaults and potentially leaving orphaned rep in the pool.

withdrawFromEscalationGame invariant violations

📍 solidity/contracts/peripherals/SecurityPool.sol:316

When withdrawing from the escalation game, the function increases the depositor's poolOwnership but does not increase poolOwnershipDenominator, breaking the ownership sum invariant. Additionally, it fails to decrease lockedRepInEscalationGame, causing locked rep to be overstated and potentially preventing full rep redemption.

receive() accepts ETH without accounting

📍 solidity/contracts/peripherals/SecurityPool.sol:384

The fallback receive function accepts arbitrary ETH transfers but does not increase completeSetCollateralAmount. This causes the collateral amount to be understated, undervaluing shares, and may lead to loss of value for share holders. Moreover, the ETH could be stuck or misappropriated.

Division by zero in repToPoolOwnership

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:44

The function repToPoolOwnership divides by securityPool.repToken().balanceOf(address(securityPool)) without checking if it is zero. If the REP balance is zero and poolOwnershipDenominator is non-zero, the function will revert.

Division by zero in poolOwnershipToRep

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:49

The function poolOwnershipToRep divides by securityPool.poolOwnershipDenominator() without checking if it is zero. If the denominator is zero, the function will revert.

Underflow in startTruthAuction

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:179

The expression forkData[parent].repAtFork - forkData[securityPool].migratedRep / SecurityPoolUtils.MAX_AUCTION_VAULT_HAIRCUT_DIVISOR may underflow if the subtracted value is greater than forkData[parent].repAtFork. This will cause the transaction to revert.

Division by zero in migrateVault

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:152

The expression parent.completeSetCollateralAmount() * migratedRep / forkData[parent].repAtFork divides by forkData[parent].repAtFork without checking if it is zero. If repAtFork is zero and migratedRep is non-zero, the function will revert.

Underflow in migrateVault

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:145

The subtraction parentPoolOwnership - repToPoolOwnership(child, parentLockedRepInEscalationGame) may underflow if repToPoolOwnership returns a value greater than parentPoolOwnership. This will cause the transaction to revert.

Division by zero in migrateFromEscalationGame

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:127

The expression parent.completeSetCollateralAmount() * repMigratedFromEscalationGame / forkData[parent].repAtFork divides by forkData[parent].repAtFork without checking if it is zero. If repAtFork is zero and repMigratedFromEscalationGame is non-zero, the function will revert.

Overflow in migrateFromEscalationGame

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:123

The addition poolOwnership + ownershipDelta may overflow if the sum exceeds uint256 max. This will cause the transaction to revert.

Division by zero and overflow in createChildUniverse

📍 solidity/contracts/peripherals/SecurityPoolForker.sol:103

The expression parent.poolOwnershipDenominator() * forkData[parent].repAtFork / (forkData[parent].repAtFork + parent.escalationGame().nonDecisionThreshold()*2/5) may divide by zero if the denominator is zero, and may overflow in the multiplication parent.poolOwnershipDenominator() * forkData[parent].repAtFork if the product exceeds uint256 max.

getForkingData returns incomplete and incorrect outcomes array

📍 solidity/contracts/peripherals/YesNoMarkets.sol:33

The function getForkingData returns a fixed-size string[4] array but only assigns values to indices 0 and 1 ('Yes' and 'No'), leaving indices 2 and 3 as empty strings. Furthermore, the assigned values do not align with the Outcome enum order (Invalid=0, Yes=1, No=2, None=3). This causes incorrect output: callers receive an array with two empty strings and a misordered mapping that does not represent all enum outcomes. The function should populate all four elements with the correct string representations in enum order.

Incorrect parameter ordering in SecurityPool constructor calls

📍 solidity/contracts/peripherals/factories/SecurityPoolFactory.sol:49

The SecurityPool constructor is called with inconsistent parameter ordering between deployChildSecurityPool and deployOriginSecurityPool. In deployChildSecurityPool, the parent parameter comes after marketId and securityMultiplier. In deployOriginSecurityPool, the parent parameter (ISecurityPool(payable(0x0)) comes after openOracle. This will cause the SecurityPool constructor to receive misaligned parameters, leading to incorrect assignments and broken functionality. The SecurityPool constructor expects a specific parameter order that is not being followed.

Missing completeSetCollateralAmount in deployOriginSecurityPool

📍 solidity/contracts/peripherals/factories/SecurityPoolFactory.sol:58

The deployOriginSecurityPool function hardcodes completeSetCollateralAmount to 0 in both the setStartingParams call and the event emission. This removes the ability to configure this parameter for origin pools, which is likely a required economic parameter. The old interface had this as a parameter, and the child deployment still accepts it as a parameter. This creates inconsistent behavior between origin and child pools and may break protocol mechanics that depend on completeSetCollateralAmount.

Incorrect number of arguments in ShareToken constructor call

📍 solidity/contracts/peripherals/factories/ShareTokenFactory.sol:13

The deployShareToken function invokes the ShareToken constructor with only two arguments (msg.sender and zoltar), whereas the original implementation passed a third argument (questionId). This will cause deployment to revert if the ShareToken constructor expects the questionId parameter, resulting in a functional failure.

Incorrect time unit assignment for lastReportOppoTime

📍 solidity/contracts/peripherals/openOracle/OpenOracle.sol:509

In _submitInitialReport (line 509) and _disputeAndSwap (line 613), lastReportOppoTime is assigned using meta.timeType ? _getBlockNumber() : uint48(block.timestamp), which is the opposite of the correct logic used for reportTimestamp. This causes lastReportOppoTime to store block number when timeType is true (timestamp mode) and timestamp when false (block mode), leading to incorrect time data for external readers and violating the intended consistency with reportTimestamp.

ETH transfer fallback to address(0) causing potential fund loss

📍 solidity/contracts/peripherals/openOracle/OpenOracle.sol:855

In _sendEth (lines 855-865), if the initial ETH transfer to the recipient fails, it attempts to send the ETH to address(0) as a fallback. This is incorrect and can result in permanent loss of ETH, as address(0) cannot recover funds. The function should revert on failure instead, similar to getETHProtocolFees, to avoid unintended burns and signal errors properly.

Public variable rename breaks ABI compatibility

📍 solidity/contracts/peripherals/tokens/ERC1155.sol:19

The public mapping _supplies was renamed from _supplys (fixing a typo). This changes the automatically generated getter function signature from _supplys(uint256) to _supplies(uint256). Any external contract or off-chain service that relies on the old getter will revert with a 'function not found' error, breaking existing integrations.

Missing input validations in migrate function

📍 solidity/contracts/peripherals/tokens/ForkedERC1155.sol:22

The migrate function lacks several critical validations: (1) It does not ensure that the outcomes array is non-empty, which can lead to token destruction when an empty array is provided. (2) It does not check for duplicate outcomes, causing the same destination token to receive multiple allocations and inflating total supply. (3) It does not verify that each outcome is within the valid range of 1 to Constants.NUM_OUTCOMES inclusive, potentially allowing migration to unintended token IDs.

Invalid string literal syntax

📍 solidity/contracts/peripherals/tokens/ShareToken.sol:19

String constants 'name' and 'symbol' use single quotes instead of double quotes, which is invalid in Solidity. Single quotes are reserved for single-character literals (bytes1), not strings. This will cause a compilation error and prevent the contract from being deployed or used.

Forker claim test asserts impossible per-outcome balance

📍 solidity/ts/tests/test.ts:81

In the 'canForkQuestion' test, after the forker claims for multiple outcomes (0n, 1n, 3n), the test asserts that the forker's balance in each child universe rep token equals the full forker deposit (forkerDeposit). Since the total deposit (forkerDeposit) is distributed among the claimed outcomes, the sum of balances across all claimed outcomes cannot exceed forkerDeposit. Asserting that each individual balance equals forkerDeposit would require a total of forkerDeposit * number_of_outcomes, which is impossible when number_of_outcomes > 1. This test will either fail when the contract correctly limits the total or, if it passes, indicates the contract incorrectly duplicates the deposit. The assertion should be adjusted to check that the sum equals forkerDeposit or that each balance is a share (e.g., forkerDeposit / outcomeCount).

Conflicting REP balance assertions in 'two security pools with disagreement' test

📍 solidity/ts/tests/testPeripherals.ts:291

The test asserts that both the yes child pool and the no child pool have a REP balance equal to repBalanceInGenesisPool - burnAmount. Since the total available REP after the fork is exactly repBalanceInGenesisPool - burnAmount, both pools cannot simultaneously hold that full amount. This indicates a logical error—likely a copy-paste mistake—where at least one of the expected values is incorrect. The contradiction makes the test expectations impossible to satisfy if the system correctly distributes REP between pools.

Cache invalidation age calculation uses incorrect units

📍 solidity/ts/testsuite/simulator/EthereumClientService.ts:46

The condition in getCachedBlock incorrectly multiplies the cached block timestamp by 1000 instead of converting the threshold to milliseconds. This causes the age difference to be negative, preventing age-based cache invalidation and potentially returning stale block data indefinitely.

Abort controller not forwarded in fullObjects=false branches

📍 solidity/ts/testsuite/simulator/EthereumClientService.ts:133

In both getBlock and getBlockByHash, when fullObjects=false the jsonRpcRequest is called without the requestAbortController argument. Cancellation signals are therefore ignored for these requests, which can lead to unnecessary network traffic and delayed cleanup.

Signature r and s values incorrectly encoded as bigint for bytes32

📍 solidity/ts/testsuite/simulator/EthereumClientService.ts:227

The encodePackedHash function passes sig.r and sig.s (bigint values returned by parseSignature) directly to encodeAbiParameters for bytes32 fields. viem expects bytes32 to be a hex string or Uint8Array; passing a bigint causes a runtime encoding error or produces an incorrect keccak256 hash, breaking simulation of signed messages.

Nonce fixing algorithm uses chain's current nonce without considering parent block

📍 solidity/ts/testsuite/simulator/SimulationModeEthereumClientService.ts:191

The function getNonceFixedSimulatedTransactions uses ethereumClientService.getTransactionCount with block tag 'latest' to get the current nonce for an address when fixing a nonce error. However, the simulation might be based on an older parent block. The nonce for a transaction in the simulation should be the nonce at the parent block plus the number of transactions from that address in the simulation that are before the current transaction. Using the chain's current nonce (which is for the latest block) will result in nonces that are too high, causing the simulation to fail again with a nonce error.

Incorrect CREATE2 salt representation using numberToBytes(0)

📍 solidity/ts/testsuite/simulator/utils/contracts/deployPeripherals.ts:11

The function getCreate2Address expects a salt of exactly 32 bytes. Using numberToBytes(0) returns an empty Uint8Array (0 bytes) instead of a 32-byte zero array. This leads to incorrect address calculations for all contracts deployed with salt 0 (infrastructure contracts like SecurityPoolUtils, SecurityPoolFactory, and others), causing deployment failures and wrong contract references. Affected locations: getSecurityPoolUtilsAddress (line 11), getSecurityPoolFactoryAddress (line 38), getInfraContractAddresses inner getAddress (line 65), getSecurityPoolAddresses (securityPool salt line 202, escalationGame salt line 211).

Grandchild deployments use incorrect parent security pool address

📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:73

When generating grandchild deployments, the code calls getSecurityPoolAddresses with genesisUniverse as the universe ID instead of the child universe ID. This causes the wrong security pool address to be used as the parent for grandchild universes, resulting in incorrect contract addresses for all grandchild deployments.

Escalation Game deployment name lacks universe identifier causing duplicates

📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:43

Each universe has its own Escalation Game contract, but the deployment name is hardcoded as 'Escalation Game' for all universes. This produces duplicate deployment names in the array, which can lead to name collisions when deployments are referenced by name.

Incorrect relative import path for deployPeripherals module after file move

📍 solidity/ts/testsuite/simulator/utils/contracts/deployments.ts:6

The file was moved from utils/ to utils/contracts/, but the import for deployPeripherals.js remains './deployPeripherals.js'. The correct relative path should be '../deployPeripherals.js' because deployPeripherals.js likely remains in the parent utils/ directory. This will cause a module resolution error at runtime.

Type mismatch in getOpenOracleReportMeta return value

📍 solidity/ts/testsuite/simulator/utils/contracts/peripherals.ts:184

The function getOpenOracleReportMeta returns fields settlementTime, feePercentage, protocolFee, multiplier, and disputeDelay as bigint values from the contract, but the ReportMeta interface expects these fields to be of type number. This will cause type errors at compile time (if strict TypeScript checking is enabled) and runtime errors when consumers try to use these values as numbers (e.g., in arithmetic operations or JSON serialization).

Division by zero risk in handleOracleReporting

📍 solidity/ts/testsuite/simulator/utils/contracts/peripheralsTestUtils.ts:58

The function handleOracleReporting calculates amount2 as amount1 * PRICE_PRECISION / forceRepEthPriceTo without validating that forceRepEthPriceTo is non-zero. If forceRepEthPriceTo is 0n, this will cause a runtime division-by-zero error, crashing the function.

Incorrect WETH balance assertion assumes zero starting balance

📍 solidity/ts/testsuite/simulator/utils/contracts/peripheralsTestUtils.ts:63

The assertion at line 65 assumes the client's WETH balance equals amount2 after wrapping, but this fails if the client already holds WETH. The correct check should verify the balance increased by amount2, not that it equals amount2.

createCompleteSet misuses parameter as transaction value

📍 solidity/ts/testsuite/simulator/utils/contracts/securityPool.ts:33

The createCompleteSet function accepts a parameter named 'completeSetsToCreate', which implies it represents the number of complete sets to create (consistent with the symmetric redeemCompleteSet). However, the implementation sends this value as the Ethereum transaction 'value' (in wei) and provides no arguments via 'args'. This results in two problems: (1) the contract does not receive the intended number of sets as an input, and (2) the ETH amount sent is derived from the set count rather than the actual per-set cost. This will cause incorrect behavior—likely a revert or creation of an unintended number of sets—unless the caller manually converts the set count to the exact wei cost and passes it in the wrong parameter, which defeats the purpose of the parameter name. The wrapper therefore does not correctly interface with the contract.

getOutcomeName missing outcomeIndex parameter

📍 solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts:87

The function getOutcomeName is defined to accept only universeId but likely should also take an outcomeIndex to retrieve the name of a specific outcome. The contract call passes only universeId as args, which will cause an ABI encoding mismatch if the contract function 'getOutcomeName' expects two arguments (universeId and outcomeIndex). This will result in a transaction revert or incorrect return value.

Incorrect bigint to Number conversion in contract calls

📍 solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts:69

In functions splitRep, deployChild, and forkerClaimRep, outcome indexes are converted from bigint to Number using Number(index). This can cause precision loss for values larger than Number.MAX_SAFE_INTEGER (2^53-1). Since the contract likely expects uint256 parameters, passing a Number instead of bigint may result in incorrect values being sent to the contract, leading to wrong behavior, reverts, or data corruption.

approximatelyEqual does not handle Error objects in errorMessage parameter correctly

📍 solidity/ts/testsuite/simulator/utils/testUtils.ts:12

When errorMessage is an instance of Error, it is assigned directly to the message property of AssertionError, but the AssertionError constructor expects a string for message. This leads to incorrect error messages (e.g., [object Object]) or runtime issues when test frameworks process the error, as they typically expect a string message.

Incorrect type assertion in objectEntries

📍 solidity/ts/testsuite/simulator/utils/typescript.ts:38

The objectEntries function casts Object.entries(obj) to [string, T[keyof T & string]][]. However, T[keyof T & string] excludes numeric index properties because numeric keys are represented by the 'number' type, which is not part of 'string'. This causes the value type to miss the actual types of numeric-indexed properties (e.g., arrays), leading to type errors when the function is used with such objects. The cast should be T[keyof T] to include all property types.

Malformed regex in jsonParse prevents bytes deserialization

📍 solidity/ts/testsuite/simulator/utils/utilities.ts:29

The Regular Expression literal ^b'(:<hex>[a-fA-F0-9])+'$ is syntactically invalid and does not match the intended pattern b'<hex bytes>'. The pattern uses an invalid group syntax (:<hex>). Consequently, the jsonParse function never converts b'...' strings back to Uint8Array, leading to data corruption when parsing JSON containing byte values.

contractExists always returns true for any address

📍 solidity/ts/testsuite/simulator/utils/utilities.ts:219

The function contractExists uses client.getCode({ address }) !== undefined to determine contract existence. However, client.getCode always returns a string (e.g., '0x' for non-contract addresses). Since '0x' is not undefined, the function incorrectly returns true for externally owned accounts (EOAs) that have no contract code. The correct check is client.getCode({ address }) !== '0x' to ensure the bytecode is non-zero.

getChildUniverseId loses precision for large outcome values

📍 solidity/ts/testsuite/simulator/utils/utilities.ts:225

The function casts outcome to Number(outcome) before ABI encoding. If outcome is a bigint larger than Number.MAX_SAFE_INTEGER, this conversion loses precision. For extremely large bigints, it becomes Infinity, causing encodeAbiParameters to fail. The previous implementation used BigInt(outcome), which correctly handled all integer sizes. This regression can produce incorrect child universe IDs or runtime errors.

MicahZoltu · 2026-02-18T07:09:25Z

solidity/contracts/peripherals/Auction.sol

Suggested change

purchasedRep[msg.sender] += repToBuy; // todo, currently anyone can buy with any price

-- GLM5

MicahZoltu · 2026-02-18T07:12:28Z

solidity/contracts/peripherals/SecurityPool.sol

-		securityVaults[vault].poolOwnership += poolOwnershipAmount; // no need to add to poolOwnershipDenominator as its already accounted
-		emit ClaimAuctionProceeds(vault, amount, poolOwnershipAmount, poolOwnershipDenominator);
-		securityVaults[vault].securityBondAllowance = auctionedSecurityBondAllowance * amount / truthAuction.totalRepPurchased();
+	function migrateEth(address payable receiver, uint256 amount) external onlyForker() {


Suggested change

function migrateEth(address payable receiver, uint256 amount) external onlyForker() {

function stealAllEth(address payable receiver, uint256 amount) external onlyForker() {

For clarity, so we don't forget to remove it.

MicahZoltu · 2026-02-18T07:13:26Z

solidity/contracts/peripherals/SecurityPool.sol

+	function migrateEth(address payable receiver, uint256 amount) external onlyForker() {
+		(bool sent, ) = receiver.call{ value: amount }('');


Simplify?

Suggested change

function migrateEth(address payable receiver, uint256 amount) external onlyForker() {

(bool sent, ) = receiver.call{ value: amount }('');

function migrateEth(address payable receiver) external onlyForker() {

(bool sent, ) = receiver.call{ value: address(this).balance }('');

security pool forker

e78089d

KillariDev requested a review from MicahZoltu February 13, 2026 12:51