zoltar

[KillariDev]

• remove market creation from zoltar
• support to fork zoltar with market with 4 outcomes

[KillariDev]

#1 This is not issue as totalTheoreticalSupply should be the max supply that we know the supply will be when the rep token is created. This number can only be reduced with burn, we will not mint above the treshold

#2 this is wrong

[MicahZoltu]

/review @Wise @security @LINTER @defender @bug @architect

Sorry to anyone who got spuriously mentioned here. I guess I need to change the handles...

[MicahZoltu]

/review

[github-actions]

🏛️ Architect

2 issues found

Hardcoded special case for GENESIS_REPUTATION_TOKEN breaks ReputationToken abstraction

📍 solidity/contracts/Zoltar.sol:82

The burnRep function directly checks the address of the genesis reputation token and applies custom logic (transfer to burn address) instead of invoking a uniform burn method. This creates a concrete dependency on a specific token address, violating the Dependency Inversion Principle and breaking the abstraction expected from the ReputationToken interface. It reduces flexibility, as future tokens with different burn semantics would require additional hardcoded checks, harming maintainability and extensibility.

Hardcoded numeric outcomes bypass enum type safety

📍 solidity/ts/testsuite/simulator/utils/deployments.ts:51

The change replaces the QuestionOutcome enum values with raw bigint literals [0n, 1n, 2n]. This introduces magic numbers, breaks consistency with getUniverseName which still uses the enum for mapping, and violates the single source of truth principle. If the enum values ever change, this hardcoded array will cause subtle bugs. The enum is already imported and used elsewhere in the same file, making this deviation particularly Fragile and architecturally misaligned with type-safe design.

[github-actions]

🛡️ Defender Against the Dark Arts

1 issue found

Removal of core functionality tests reduces security coverage

📍 solidity/ts/tests/test.ts

The diff deletes three critical test cases: 'canCreateQuestion', 'canResolveQuestion', and 'canInitialReport'. These tests validated the question lifecycle, including creation, reporting, and dispute handling. Their absence significantly reduces test coverage for essential features, potentially concealing backdoors or malicious behavior in those areas. This pattern is a strong insider threat indicator.

[github-actions]

🔧 Expensive Linter

9 issues found

Typographical error in SPDX license identifier

📍 solidity/contracts/Constants.sol:1

The SPDX license identifier 'UNICENSE' is misspelled. The correct identifier according to SPDX specifications is 'Unlicense' (capital U, rest lowercase). This is a critical typographical error that affects license compliance recognition.

Inconsistent indentation in function declaration

📍 solidity/contracts/ReputationToken.sol:36

The function getTotalTheoreticalSupply uses spaces for indentation (four spaces) instead of tabs, which is inconsistent with the rest of the file where all other declarations and blocks use tabs.

Inconsistent string literal quoting and error message capitalization

📍 solidity/contracts/Zoltar.sol:67

The new code uses single quotes for string literals, while the existing codebase uses double quotes. Additionally, several require/return messages start with lowercase letters, and the message on line 68 contains a typo ('atleast'). These reduce code consistency and readability. The following lines should be updated to use double quotes and proper capitalization, fixing the typo as well.

Inconsistent spacing around division operator

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

On line 56, the division operator '/' lacks spaces around it, while on line 64, spaces are used around '/' and '-'. For consistency, binary operators should have spaces on both sides.

Inconsistent use of semicolons in expression statements

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

Assert.ok statements on lines 79 and 97 have explicit semicolons, while other assert statements in the file do not use semicolons. To maintain consistent style, semicolons should be omitted where not required.

Inconsistent parameter naming in forkerClaimRep

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

The parameter 'outcomeindexes' in the forkerClaimRep function uses a lowercase 'i', while the similar parameter in splitRep is named 'outcomeIndexes' with uppercase 'I'. This inconsistency violates common naming conventions (camelCase) and may cause confusion during maintenance.

Missing trailing comma in multiline object literal

📍 solidity/ts/testsuite/simulator/utils/deployPeripherals.ts:168

The object literal in the writeContract call within deployOriginSecurityPool does not have a trailing comma on the last property (args), while other multiline object literals in the same file consistently use trailing commas on all properties, including the last one. This creates a stylistic inconsistency in code formatting.

Misspelled variable name 'oucomes'

📍 solidity/ts/testsuite/simulator/utils/deployments.ts:51

The variable 'oucomes' is consistently misspelled across lines 51, 54, and 64 and should be corrected to 'outcomes' to adhere to standard naming conventions and avoid confusion.

Hardcoded numeric values instead of QuestionOutcome enum

📍 solidity/ts/testsuite/simulator/utils/deployments.ts:51

Line 51 uses hardcoded bigint literals [0n, 1n, 2n] instead of the imported QuestionOutcome enum [QuestionOutcome.Invalid, QuestionOutcome.No, QuestionOutcome.Yes]. This is inconsistent with the rest of the file (e.g., line 15) where QuestionOutcome is used for semantic clarity, reducing readability and maintainability.

[github-actions]

🧠 Wise Man

12 issues found

Inconsistent naming between state variable and setter function

📍 solidity/contracts/ReputationToken.sol:21

The state variable is named totalTheoreticalSupply while the setter function is named setMaxTheoreticalSupply. The discrepancy between 'total' and 'max' creates confusion about the intended semantics. For consistency, either rename the variable to maxTheoreticalSupply or rename the setter to setTotalTheoreticalSupply. Given the existence of a getter getTotalTheoreticalSupply, aligning the setter to setTotalTheoreticalSupply is recommended.

Setter function lacks event emission for state change

📍 solidity/contracts/ReputationToken.sol:10

The setMaxTheoreticalSupply function modifies totalTheoreticalSupply without emitting any event. Emitting an event is a best practice for state-modifying functions to allow off-chain tracking. Add a custom event SetTotalTheoreticalSupply that logs the old and new values, and emit it inside the setter.

Typo in variable name 'priorRepbalance'

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

The variable name 'priorRepbalance' on line 51 is misspelled; it should be 'priorRepBalance' for consistency and to avoid confusion. This typo reduces readability and may lead to errors in maintenance.

Magic numbers in forker deposit calculation

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

The calculation for 'forkerDeposit' on line 64 uses magic numbers (20n and 5n) without descriptive context. Extract this logic into a well-named helper function or constant to clarify intent (e.g., 4% deposit after burns) and improve maintainability.

Overly complex test function 'canForkQuestion'

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

The test function (lines 33-101) covers multiple behaviors (forking, claiming, splitting) in a single test, violating the single-responsibility principle for tests. This reduces clarity, makes failures harder to diagnose, and complicates maintenance. Split into separate, focused test functions.

Incorrect salt format for CREATE2 address calculation

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

In getZoltarAddress, the salt parameter uses numberToBytes(0), which may not produce a 32-byte value required by the CREATE2 opcode. This can lead to an incorrect contract address. Use bytes32String(0n) to ensure a proper 32-byte zero salt.

Unnecessary conversion of bigint to number in write operations

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

In splitRep, deployChild, and forkerClaimRep, bigint arguments are converted to Number when calling writeContract. This can cause precision loss for large integers and is inconsistent with other functions that use bigint directly. Pass bigint values without conversion to ensure correctness and consistency.

Fix typographical error 'eventhought' in error messages

📍 solidity/ts/testsuite/simulator/utils/deployPeripherals.ts:80

The error messages in ensureInfraDeployed contain the misspelled word 'eventhought' which should be 'even though'. This reduces readability and professionalism. All occurrences should be corrected.

Refactor duplicated deployment logic in ensureInfraDeployed

📍 solidity/ts/testsuite/simulator/utils/deployPeripherals.ts:78

The ensureInfraDeployed function contains repeated code for checking and deploying each contract. This duplication increases maintenance burden and risk of inconsistencies. A data-driven approach using an array of deployment descriptors would make the code more concise and easier to extend.

Variable name misspelled as 'oucomes'

📍 solidity/ts/testsuite/simulator/utils/deployments.ts:51

The variable 'oucomes' is consistently misspelled across multiple lines. It should be 'outcomes' to correct the spelling, aligning with common English and improving code readability and maintainability.

Magic numbers used instead of named constants for outcomes

📍 solidity/ts/testsuite/simulator/utils/deployments.ts:51

The array [0n, 1n, 2n] represents specific outcomes (Invalid, No, Yes) but uses magic numbers, reducing code clarity. Replace with the corresponding QuestionOutcome enum values (QuestionOutcome.Invalid, QuestionOutcome.No, QuestionOutcome.Yes) to make the intent explicit and improve long-term maintainability.

getChildUniverseId throws TypeError for bigint outcome

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

The function now accepts an outcome as either QuestionOutcome or bigint, but the implementation unconditionally calls BigInt(outcome). This throws a TypeError when outcome is already a bigint, as BigInt cannot be applied to a bigint. The function should check the type and only convert when needed.

[github-actions]

🐛 Bug Hunter

9 issues found

Mint function does not update totalTheoreticalSupply

📍 solidity/contracts/ReputationToken.sol:26

The mint function mints tokens via _mint but does not increment the totalTheoreticalSupply variable. This leads to an inaccurate value returned by getTotalTheoreticalSupply, as it does not account for newly minted tokens. The burn function correctly decrements totalTheoreticalSupply, creating an inconsistent state where totalTheoreticalSupply does not reflect the net effect of mints and burns, violating the intended tracking of theoretical supply.

splitRepInternal mints duplicate REP for multiple outcomes, breaking token conservation

📍 solidity/contracts/Zoltar.sol:124

In splitRepInternal, the same amount is minted to the recipient in each child universe for the provided outcomeIndexes. However, this amount is only burned once from the parent universe (via burnRep). This causes the total REP supply to increase by amount * (outcomeIndexes.length - 1) on each call. For example, if 100 REP is split for 2 outcomes, 100 REP is burned from the parent but 200 REP is minted in child universes, increasing total supply by 100. This affects both splitRep and forkerClaimRep.

getForkThreshold reverts with low-level error for non-existent universe

📍 solidity/contracts/Zoltar.sol:60

When called with a universeId that does not exist (i.e., not stored in the universes mapping), getForkThreshold attempts to call getTotalTheoreticalSupply on address(0), causing a revert with a low-level 'call to non-contract' error instead of a clear message. This should explicitly check for universe existence first.

Incorrect address comparison in canDeployContract test

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

The assertion on line 30 compares genesisUniverseData.reputationToken (string) directly to GENESIS_REPUTATION_TOKEN (bigint) without converting the constant to an address string using addressString() and without normalizing case. This will always fail due to type mismatch and potential case differences, causing the test to incorrectly fail. The original test correctly used addressString(GENESIS_REPUTATION_TOKEN) and .toLowerCase().

Incorrect BigInt conversion in getUniverseData

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

The forkingOutcomeIndex value returned by the contract read is a bigint (as viem returns bigints for uint256). Calling BigInt() on an existing bigint throws a TypeError, causing the function to crash. The conversion is unnecessary and should be removed.

Incorrect error message for securityPoolFactory deployment

📍 solidity/ts/testsuite/simulator/utils/deployPeripherals.ts:104

When securityPoolFactory deployment fails, the error thrown incorrectly references 'priceOracleManagerAndOperatorQueuerFactory' instead of 'securityPoolFactory', and contains a typo ('eventhought'). This misleads debugging.

Typographical errors in ensureInfraDeployed error messages

📍 solidity/ts/testsuite/simulator/utils/deployPeripherals.ts:80

All error messages in ensureInfraDeployed contain the misspelling 'eventhought' which should be 'even though'.

Incorrect universe ID used for grandchild security pool address lookup

📍 solidity/ts/testsuite/simulator/utils/deployments.ts:64

In the getDeployments function, the grandchild deployment generation (line 64) incorrectly passes genesisUniverse as the universeId argument to getSecurityPoolAddresses. It should instead use the child universe ID corresponding to the current outcome. This results in computing security pool addresses based on the wrong universe, leading to incorrect deployments for all grandchild universes.

getChildUniverseId crashes with bigint outcome

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

The function uses BigInt(outcome) which throws a TypeError when outcome is already a bigint. The updated type signature allows bigint, but the implementation does not handle this case, causing runtime crashes.

[github-actions]

🔒 Security Reviewer

7 issues found

Invalid SPDX License Identifier

📍 solidity/contracts/Constants.sol:1

The SPDX license identifier 'Unlicense' is invalid; the correct identifier is 'UNLICENSE' (uppercase) per the SPDX specification. Using an invalid identifier may cause license compliance tools to misinterpret the license terms, potentially leading to legal or distribution risks.

Underflow vulnerability in totalTheoreticalSupply decrement

📍 solidity/contracts/ReputationToken.sol:32

In the burn function, totalTheoreticalSupply is decremented without checking that value <= totalTheoreticalSupply. While Solidity 0.8.x reverts on underflow, this will cause legitimate burns to fail if totalTheoreticalSupply is less than the burn amount, potentially locking funds. Since totalTheoreticalSupply is independently managed and not automatically adjusted during mints, it can become desynchronized from actual token supply, leading to arbitrary burn failures.

Incorrect REP splitting leading to inflation and loss of funds

📍 solidity/contracts/Zoltar.sol:124

The splitRepInternal function burns a specified amount of REP from the parent universe but mints the full amount for each provided outcomeIndex without dividing. This leads to inflation if multiple outcomeIndexes are provided. Additionally, if an empty outcomeIndexes array is provided, the function burns the amount without minting any REP, causing loss of funds. Both issues stem from improper handling of the outcomeIndexes parameter.

deployChild can overwrite existing child universes

📍 solidity/contracts/Zoltar.sol:106

The deployChild function does not check whether a child universe already exists before deploying a new ReputationToken contract. Since the child universe ID is derived from the parent and outcomeIndex, calling deployChild for an existing child will deploy a new token contract at the same address (due to same salt), overwriting the existing one and destroying its state. This allows any attacker to destroy existing child universes.

Incorrect salt length for CREATE2 address calculation

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

In getZoltarAddress, the salt for CREATE2 is computed using numberToBytes(0), which may not produce a 32-byte value as required. CREATE2 salt must be exactly 32 bytes; using a shorter salt can lead to incorrect address derivation, causing deployments to fail or contracts to be deployed at unintended addresses.

Unsafe conversion of bigint to Number in contract calls

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

In splitRep, deployChild, and forkerClaimRep functions, outcome indexes are converted from bigint to Number using Number(). Since Ethereum uint256 can exceed Number.MAX_SAFE_INTEGER (2^53 - 1), this conversion can lose precision, resulting in incorrect indexes being passed to the contract. This can lead to state inconsistencies, incorrect reputation splits, or potential exploits if an attacker provides large indexes.

Inconsistent outcome representation breaks security pool address derivation

📍 solidity/ts/testsuite/simulator/utils/deployments.ts:51

The code mixes two different representations for question outcomes: the QuestionOutcome enum (used in getUniverseName) and raw bigint literals [0n, 1n, 2n] (used in deployment generation). This causes getChildUniverseId to receive raw bigints while getUniverseName derives outcome names by subtracting 1 from the branch index and mapping to enum values. The mismatch between these representations (raw bigint vs. enum-indexed minus one) creates inconsistent universe IDs and deployment addresses, leading to Security Pools being deployed for incorrect outcomes. This breaks the fundamental universe/outcome hierarchy and could cause funds to be sent to unintended contracts.