add escalation game with not enough tests

[KillariDev]

No description provided.

[github-actions]

I've Reviewed the Code

Introduces new EscalationGame contracts and TypeScript test utilities implementing an attrition-based escalation mechanism, but contains critical logic errors in deposit excess calculations, improper tie-handling in market resolution, and a hardcoded CREATE2 salt preventing multiple deployments.

⚠️ 6 issues found across 6 files

#1 Incorrect deposit excess calculation and condition ordering in claimDepositForWinning

solidity/contracts/peripherals/EscalationGame.sol L175-L194

The function claimDepositForWinning contains two logic errors in handling deposits that straddle the binding capital threshold:

1. Line 181 uses if (deposit.cumulativeAmount > maxWithdrawableBalance) which incorrectly captures deposits where only the portion exceeds the threshold (straddling deposits), treating them as fully excess deposits. This prevents such deposits from receiving the proportional rewards calculated in the else-if branch.

2. Line 184 checks deposit.cumulativeAmount + deposit.amount > maxWithdrawableBalance but cumulativeAmount already includes deposit.amount, causing double counting. Line 185 then calculates excess = deposit.cumulativeAmount + deposit.amount - maxWithdrawableBalance which results in an incorrect excess value (larger than the actual excess by exactly deposit.amount).

3. In the straddling case, if the calculated excess exceeds deposit.amount (which occurs due to the double counting), the subtraction deposit.amount - excess on line 187 will underflow. While Solidity 0.8.x will revert on underflow, this renders the straddling logic completely broken.

The correct logic should check if the previous cumulative (cumulativeAmount - deposit.amount) was already above the max to identify fully excess deposits, then check for straddling, and calculate excess as deposit.cumulativeAmount - maxWithdrawableBalance.
Tags: bug, security
Affected code:

175: 	function claimDepositForWinning(uint256 depositIndex, YesNoMarkets.Outcome outcome) public returns (address depositor, uint256 amountToWithdraw) {
176: 		require(msg.sender == address(securityPool) || msg.sender == address(securityPool.securityPoolForker()), 'Only Security Pool can withdraw');
177: 		Deposit memory deposit = deposits[uint8(outcome)][depositIndex];
178: 		deposits[uint8(outcome)][depositIndex].amount = 0;
179: 		depositor = deposit.depositor;
180: 		uint256 maxWithdrawableBalance = getBindingCapital();
181: 		if (deposit.cumulativeAmount > maxWithdrawableBalance) {
182: 			amountToWithdraw = deposit.amount;
183: 			emit ClaimDeposit(amountToWithdraw, 0);
184: 		} else if (deposit.cumulativeAmount + deposit.amount > maxWithdrawableBalance) {
185: 			uint256 excess = (deposit.cumulativeAmount + deposit.amount - maxWithdrawableBalance);
186: 			uint256 burnAmount = excess * 2 / 5;
187: 			amountToWithdraw = (deposit.amount - excess) + excess * 2 - burnAmount;
188: 			emit ClaimDeposit(amountToWithdraw, burnAmount);
189: 		} else {
190: 			uint256 burnAmount = (deposit.amount * 2) / 5;
191: 			amountToWithdraw = deposit.amount * 2 - burnAmount;
192: 			emit ClaimDeposit(amountToWithdraw, burnAmount);
193: 		}
194: 	}

Proposed change:

	function claimDepositForWinning(uint256 depositIndex, YesNoMarkets.Outcome outcome) public returns (address depositor, uint256 amountToWithdraw) {
		require(msg.sender == address(securityPool) || msg.sender == address(securityPool.securityPoolForker()), 'Only Security Pool can withdraw');
		Deposit memory deposit = deposits[uint8(outcome)][depositIndex];
		deposits[uint8(outcome)][depositIndex].amount = 0;
		depositor = deposit.depositor;
		uint256 maxWithdrawableBalance = getBindingCapital();
		uint256 prevCumulative = deposit.cumulativeAmount - deposit.amount;
		if (prevCumulative >= maxWithdrawableBalance) {
			amountToWithdraw = deposit.amount;
			emit ClaimDeposit(amountToWithdraw, 0);
		} else if (deposit.cumulativeAmount > maxWithdrawableBalance) {
			uint256 excess = deposit.cumulativeAmount - maxWithdrawableBalance;
			uint256 burnAmount = excess * 2 / 5;
			amountToWithdraw = (deposit.amount - excess) + excess * 2 - burnAmount;
			emit ClaimDeposit(amountToWithdraw, burnAmount);
		} else {
			uint256 burnAmount = (deposit.amount * 2) / 5;
			amountToWithdraw = deposit.amount * 2 - burnAmount;
			emit ClaimDeposit(amountToWithdraw, burnAmount);
		}
	}

#2 Improper tie handling in getMarketResolution

solidity/contracts/peripherals/EscalationGame.sol L119-L130

The getMarketResolution function uses a series of comparative checks (lines 127-129) that do not properly handle tie scenarios between outcome balances. If two or more outcomes have equal highest balances (e.g., balances[1] == balances[2] > balances[0]), the function arbitrarily returns the outcome with the higher index (No in this example) rather than correctly identifying the tie and returning Outcome.None.

This violates the intended game logic where ties should result in an undecided/None state, potentially causing incorrect market resolution and unfair distribution of stakes. The TODO comment on line 126 acknowledges this issue but it represents a critical flaw in the dispute resolution mechanism.
Tags: bug, logic
Affected code:

119: 	function getMarketResolution() public view returns (YesNoMarkets.Outcome outcome){
120: 		uint256 currentTotalCost = totalCost();
121: 		uint8 invalidOver = balances[0] >= currentTotalCost ? 1 : 0;
122: 		uint8 yesOver = balances[1] >= currentTotalCost ? 1 : 0;
123: 		uint8 noOver = balances[2] >= currentTotalCost ? 1 : 0;
124: 		if (invalidOver + yesOver + noOver >= 2) return YesNoMarkets.Outcome.None; // if two or more outcomes are over the total cost, the game is still going
125: 		// the game has ended due to timeout
126: 		// TODO, doesn't handle ties well logically. We could avoid it by checking if tie is happening before deposit and break it there
127: 		if (balances[0] > balances[1] && balances[0] > balances[2]) return YesNoMarkets.Outcome.Invalid;
128: 		if (balances[1] > balances[0] && balances[1] > balances[2]) return YesNoMarkets.Outcome.Yes;
129: 		return YesNoMarkets.Outcome.No;
130: 	}

Proposed change:

	function getMarketResolution() public view returns (YesNoMarkets.Outcome outcome){
		uint256 currentTotalCost = totalCost();
		uint8 invalidOver = balances[0] >= currentTotalCost ? 1 : 0;
		uint8 yesOver = balances[1] >= currentTotalCost ? 1 : 0;
		uint8 noOver = balances[2] >= currentTotalCost ? 1 : 0;
		if (invalidOver + yesOver + noOver >= 2) return YesNoMarkets.Outcome.None;
		
		uint256 maxBalance = balances[0];
		outcome = YesNoMarkets.Outcome.Invalid;
		if (balances[1] > maxBalance) {
			maxBalance = balances[1];
			outcome = YesNoMarkets.Outcome.Yes;
		} else if (balances[1] == maxBalance) {
			return YesNoMarkets.Outcome.None;
		}
		if (balances[2] > maxBalance) {
			return YesNoMarkets.Outcome.No;
		} else if (balances[2] == maxBalance) {
			return YesNoMarkets.Outcome.None;
		}
	}

#3 Hardcoded CREATE2 salt prevents multiple deployments from same caller

solidity/contracts/peripherals/factories/EscalationGameFactory.sol L10

The CREATE2 salt is hardcoded to bytes32(0). Since the deployment address is deterministic based on salt and init code (which includes msg.sender via the constructor argument), any subsequent call from the same msg.sender will attempt to deploy to the same address, causing the transaction to revert if a contract already exists there. This prevents creating multiple games over time or replacing a destroyed game. The salt should be unique per deployment (e.g., derived from a factory nonce or passed as a parameter).
Tags: bug, architecture
Affected code:

10: 		EscalationGame game = new EscalationGame{ salt: bytes32(uint256(0x0)) }(securityPool);

#4 Incorrect import source for assert module

solidity/ts/tests/testEscalationGame.ts L7

The code imports assert from node:test, but Node.js's test runner module does not export an assert object. The assertion library is provided by the separate node:assert module. This will cause a runtime error or undefined behavior when the test attempts to use assertion methods.
Tags: bug, compatibility
Affected code:

7: import assert from 'node:assert'

Proposed change:

import assert from 'node:assert'

#5 Misleading assertion message

solidity/ts/tests/testEscalationGame.ts L35

The assertion message 'game was started' describes a positive state that should be true, but when the assertion fails (startingTime is 0n), the error message implies the game was successfully started when it was not. This creates confusion during debugging. The message should describe the expectation, such as 'starting time should not be zero' or 'game should be started'.
Tags: language, maintainability
Affected code:

35: 		assert.strictEqual(startingTime !== 0n, true, 'game was started')

Proposed change:

assert.notStrictEqual(startingTime, 0n, 'starting time should not be zero')

#6 Incorrect salt encoding for CREATE2 address calculation

solidity/ts/testsuite/simulator/utils/contracts/escalationGame.ts L66

The deployEscalationGame function uses numberToBytes(0) to generate the salt for getCreate2Address. By default, numberToBytes returns the minimum byte size required to represent the number (1 byte for 0), whereas CREATE2 salts must be 32 bytes (bytes32). This causes an address mismatch between the calculated address and the actual deployed contract address, as the factory likely uses a 32-byte salt.
Tags: bug, security
Affected code:

66: 		salt: numberToBytes(0)

Proposed change:

		salt: numberToBytes(0, { size: 32 })

[KillariDev]

others are not issues