Skip to content

zoltar#51

Open
KillariDev wants to merge 2 commits intomainfrom
zoltar
Open

zoltar#51
KillariDev wants to merge 2 commits intomainfrom
zoltar

Conversation

@KillariDev
Copy link
Collaborator

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

@github-actions

This comment has been minimized.

@github-actions
Copy link

I've Reviewed the Code

The changes introduce totalTheoreticalSupply tracking and an isZoltar access modifier to the ReputationToken contract while significantly refactoring the test suite to focus on universe forking scenarios, but contain critical logic errors in the asymmetric supply update mechanism and incorrect CREATE2 salt encoding that result in invalid contract address calculations.

⚠️ 2 issues found across 4 files

#1 Logic error: Burn function incorrectly decrements totalTheoreticalSupply

solidity/contracts/ReputationToken.sol L30-L34

The burn function decrements totalTheoreticalSupply (line 32), but the mint function does not increment it, creating an asymmetric state update. Furthermore, the setter is named setMaxTheoreticalSupply suggesting this variable represents a maximum cap, which should not decrease when tokens are burned. This creates an underflow risk (revert in Solidity 0.8+) if burn is called with a value exceeding the current totalTheoreticalSupply, effectively preventing token burns in certain states and breaking the expected economic model.
Tags: bug, logic, naming
Affected code:

30: 	function burn(address account, uint256 value) external isZoltar {
31: 		_burn(account, value);
32: 		totalTheoreticalSupply -= value;
33: 		emit Burn(account, value);
34: 	}

Proposed change:

	function burn(address account, uint256 value) external isZoltar {
		_burn(account, value);
		emit Burn(account, value);
	}

#2 Incorrect CREATE2 salt encoding in getZoltarAddress

solidity/ts/testsuite/simulator/utils/contracts/zoltar.ts L9-L12

The function uses numberToBytes(0) which produces a single byte (0x00) for the salt parameter. Ethereum's CREATE2 opcode requires a strictly 32-byte salt. This causes the computed contract address to be incorrect compared to the standard deployment which uses 32-byte salts (as evidenced by getRepTokenAddress correctly using bytes32String). This will cause isZoltarDeployed to always return false and getZoltarAddress to return the wrong address.
Tags: bug, security
Affected code:

9: export function getZoltarAddress() {
10: 	const bytecode: `0x${ string }` = `0x${ Zoltar_Zoltar.evm.bytecode.object }`
11: 	return getContractAddress({ bytecode, from: addressString(PROXY_DEPLOYER_ADDRESS), opcode: 'CREATE2', salt: numberToBytes(0) })
12: }

Proposed change:

export function getZoltarAddress() {
	const bytecode: `0x${ string }` = `0x${ Zoltar_Zoltar.evm.bytecode.object }`
	return getContractAddress({ bytecode, from: addressString(PROXY_DEPLOYER_ADDRESS), opcode: 'CREATE2', salt: bytes32String(0n) })
}

@KillariDev
Copy link
Collaborator Author

#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant