From 65b82b48a10610de22a5480ba7e25a0d5022fbe7 Mon Sep 17 00:00:00 2001 From: hudsonhrh Date: Fri, 6 Feb 2026 18:36:36 -0500 Subject: [PATCH] feat: Implement 11 security fixes and add comprehensive test coverage - Fix 1: Add replay protection to announceWinner in voting contracts - Fix 2: Enforce strict access control for Executor.setCaller - Fix 3: Restrict OrgDeployer.setUniversalPasskeyFactory to poaManager - Fix 4: Add access control to PaymasterHub.registerOrgWithVoucher - Fix 5: Fix PaymasterHub._checkOrgBalance underflow logic - Fix 6: Move bounty totalPaid increment to post-transfer in PaymasterHub - Fix 7: Add nonReentrant guard and fix CEI violation in claimVouchedHat - Fix 8: Remove unsafe dailyVouchCount decrement in vouch revocation - Fix 9: Add configurable SELF_REVIEW permission for task self-approval - Fix 10: Salt answer hashes with module ID to prevent brute-force - Fix 11: Validate zero public keys and max credentials in PasskeyAccount Added 19 comprehensive tests across 7 test files covering all security fixes. All 646 tests pass with zero failures. Co-Authored-By: Claude Haiku 4.5 --- src/DirectDemocracyVoting.sol | 9 ++- src/EducationHub.sol | 4 +- src/EligibilityModule.sol | 17 ++--- src/Executor.sol | 5 +- src/HybridVoting.sol | 1 + src/OrgDeployer.sol | 8 +-- src/PasskeyAccount.sol | 8 +++ src/PaymasterHub.sol | 37 ++++++---- src/TaskManager.sol | 12 +++- src/libs/HybridVotingCore.sol | 8 ++- src/libs/TaskPerm.sol | 1 + src/libs/VotingErrors.sol | 1 + test/DeployerTest.t.sol | 37 ++++++++++ test/DirectDemocracyVoting.t.sol | 28 ++++++++ test/Executor.t.sol | 19 +++++ test/HybridVoting.t.sol | 20 ++++++ test/Passkey.t.sol | 62 +++++++++++++++++ test/PasskeyPaymasterIntegration.t.sol | 9 +-- test/PaymasterHubSolidarity.t.sol | 45 +++++++++++- test/TaskManager.t.sol | 96 ++++++++++++++++++++++++++ 20 files changed, 383 insertions(+), 44 deletions(-) diff --git a/src/DirectDemocracyVoting.sol b/src/DirectDemocracyVoting.sol index a009ca7..f4bbe9b 100644 --- a/src/DirectDemocracyVoting.sol +++ b/src/DirectDemocracyVoting.sol @@ -46,6 +46,7 @@ contract DirectDemocracyVoting is Initializable { uint256[] pollHatIds; // array of specific hat IDs for this poll bool restricted; // if true only allowedHats can vote mapping(uint256 => bool) pollHatAllowed; // O(1) lookup for poll hat permission + bool executed; // finalization guard } /* ─────────── ERC-7201 Storage ─────────── */ @@ -408,9 +409,13 @@ contract DirectDemocracyVoting is Initializable { whenNotPaused returns (uint256 winner, bool valid) { - (winner, valid) = _calcWinner(id); Layout storage l = _layout(); - IExecutor.Call[] storage batch = l._proposals[id].batches[winner]; + Proposal storage prop = l._proposals[id]; + if (prop.executed) revert VotingErrors.AlreadyExecuted(); + prop.executed = true; + + (winner, valid) = _calcWinner(id); + IExecutor.Call[] storage batch = prop.batches[winner]; if (valid && batch.length > 0) { uint256 len = batch.length; diff --git a/src/EducationHub.sol b/src/EducationHub.sol index 2cc2ff6..50d4edb 100644 --- a/src/EducationHub.sol +++ b/src/EducationHub.sol @@ -198,7 +198,7 @@ contract EducationHub is Initializable, ContextUpgradeable, ReentrancyGuardUpgra } l._modules[id] = - Module({answerHash: keccak256(abi.encodePacked(correctAnswer)), payout: uint128(payout), exists: true}); + Module({answerHash: keccak256(abi.encodePacked(id, correctAnswer)), payout: uint128(payout), exists: true}); emit ModuleCreated(id, title, contentHash, payout); } @@ -229,7 +229,7 @@ contract EducationHub is Initializable, ContextUpgradeable, ReentrancyGuardUpgra Layout storage l = _layout(); Module storage m = _module(l, id); if (_isCompleted(l, _msgSender(), id)) revert AlreadyCompleted(); - if (keccak256(abi.encodePacked(answer)) != m.answerHash) revert InvalidAnswer(); + if (keccak256(abi.encodePacked(uint48(id), answer)) != m.answerHash) revert InvalidAnswer(); l.token.mint(_msgSender(), m.payout); _setCompleted(l, _msgSender(), id); diff --git a/src/EligibilityModule.sol b/src/EligibilityModule.sol index 04409f1..4b055e5 100644 --- a/src/EligibilityModule.sol +++ b/src/EligibilityModule.sol @@ -111,7 +111,7 @@ contract EligibilityModule is Initializable, IHatsEligibility { uint256 private _notEntered = 1; modifier nonReentrant() { - require(_notEntered == 1, "ReentrancyGuard: reentrant call"); + require(_notEntered != 2, "ReentrancyGuard: reentrant call"); _notEntered = 2; _; _notEntered = 1; @@ -197,6 +197,8 @@ contract EligibilityModule is Initializable, IHatsEligibility { function initialize(address _superAdmin, address _hats, address _toggleModule) external initializer { if (_superAdmin == address(0) || _hats == address(0)) revert ZeroAddress(); + _notEntered = 1; + Layout storage l = _layout(); l.superAdmin = _superAdmin; l.hats = IHats(_hats); @@ -768,9 +770,8 @@ contract EligibilityModule is Initializable, IHatsEligibility { uint32 newCount = l.currentVouchCount[hatId][wearer] - 1; l.currentVouchCount[hatId][wearer] = newCount; - uint256 currentDay = block.timestamp / SECONDS_PER_DAY; - uint32 dailyCount = l.dailyVouchCount[msg.sender][currentDay] - 1; - l.dailyVouchCount[msg.sender][currentDay] = dailyCount; + // Note: dailyVouchCount is NOT decremented on revocation. + // It's a rate limiter only — revoking doesn't give back vouch slots. emit VouchRevoked(msg.sender, wearer, hatId, newCount); @@ -795,7 +796,7 @@ contract EligibilityModule is Initializable, IHatsEligibility { * The EligibilityModule contract mints the hat using its ELIGIBILITY_ADMIN permissions. * @param hatId The ID of the hat to claim */ - function claimVouchedHat(uint256 hatId) external whenNotPaused { + function claimVouchedHat(uint256 hatId) external whenNotPaused nonReentrant { Layout storage l = _layout(); // Check if caller is eligible to claim this hat @@ -805,13 +806,13 @@ contract EligibilityModule is Initializable, IHatsEligibility { // Check if already wearing the hat require(!l.hats.isWearerOfHat(msg.sender, hatId), "Already wearing hat"); + // State change BEFORE external call (CEI pattern) + delete l.roleApplications[hatId][msg.sender]; + // Mint the hat to the caller using EligibilityModule's admin powers bool success = l.hats.mintHat(hatId, msg.sender); require(success, "Hat minting failed"); - // Clean up any pending role application - delete l.roleApplications[hatId][msg.sender]; - emit HatClaimed(msg.sender, hatId); } diff --git a/src/Executor.sol b/src/Executor.sol index c7da034..b1b6b99 100644 --- a/src/Executor.sol +++ b/src/Executor.sol @@ -78,10 +78,7 @@ contract Executor is Initializable, OwnableUpgradeable, PausableUpgradeable, Ree function setCaller(address newCaller) external { if (newCaller == address(0)) revert ZeroAddress(); Layout storage l = _layout(); - if (l.allowedCaller != address(0)) { - // After first set, only current caller or owner can change - if (msg.sender != l.allowedCaller && msg.sender != owner()) revert UnauthorizedCaller(); - } + if (msg.sender != l.allowedCaller && msg.sender != owner()) revert UnauthorizedCaller(); l.allowedCaller = newCaller; emit CallerSet(newCaller); } diff --git a/src/HybridVoting.sol b/src/HybridVoting.sol index 9cc0ac5..15aa819 100644 --- a/src/HybridVoting.sol +++ b/src/HybridVoting.sol @@ -52,6 +52,7 @@ contract HybridVoting is Initializable { bool restricted; // if true only pollHatIds can vote mapping(uint256 => bool) pollHatAllowed; // O(1) lookup for poll hat permission ClassConfig[] classesSnapshot; // Snapshot the class config to freeze semantics for this proposal + bool executed; // finalization guard } /* ─────── ERC-7201 Storage ─────── */ diff --git a/src/OrgDeployer.sol b/src/OrgDeployer.sol index 2c11cf2..2d522da 100644 --- a/src/OrgDeployer.sol +++ b/src/OrgDeployer.sol @@ -174,14 +174,12 @@ contract OrgDeployer is Initializable { /** * @notice Set the universal passkey factory address - * @dev Callable by PoaManager, or anyone for one-time initial setup (when factory is not yet set) + * @dev Only callable by PoaManager */ function setUniversalPasskeyFactory(address _universalFactory) external { Layout storage l = _layout(); - // Allow one-time setup by anyone (when factory is not yet set), or require PoaManager for updates - if (l.universalPasskeyFactory != address(0) && msg.sender != l.poaManager) { - revert InvalidAddress(); - } + if (msg.sender != l.poaManager) revert InvalidAddress(); + if (_universalFactory == address(0)) revert InvalidAddress(); l.universalPasskeyFactory = _universalFactory; } diff --git a/src/PasskeyAccount.sol b/src/PasskeyAccount.sol index 09e2f69..4745950 100644 --- a/src/PasskeyAccount.sol +++ b/src/PasskeyAccount.sol @@ -236,6 +236,7 @@ contract PasskeyAccount is Initializable, IAccount, IPasskeyAccount { /// @inheritdoc IPasskeyAccount function addCredential(bytes32 credentialId, bytes32 pubKeyX, bytes32 pubKeyY) external override onlySelf { + if (pubKeyX == bytes32(0) || pubKeyY == bytes32(0)) revert InvalidSignature(); Layout storage l = _layout(); // Check if credential already exists @@ -319,6 +320,7 @@ contract PasskeyAccount is Initializable, IAccount, IPasskeyAccount { /// @inheritdoc IPasskeyAccount function initiateRecovery(bytes32 credentialId, bytes32 pubKeyX, bytes32 pubKeyY) external override onlyGuardian { + if (pubKeyX == bytes32(0) || pubKeyY == bytes32(0)) revert InvalidSignature(); Layout storage l = _layout(); // Generate recovery ID @@ -361,6 +363,12 @@ contract PasskeyAccount is Initializable, IAccount, IPasskeyAccount { revert RecoveryDelayNotPassed(); } + // Check max credentials limit before adding + uint8 maxCreds = _getMaxCredentials(); + if (l.credentialIds.length >= maxCreds) { + revert MaxCredentialsReached(); + } + // Add the new credential bytes32 credentialId = request.credentialId; diff --git a/src/PaymasterHub.sol b/src/PaymasterHub.sol index bbb4648..6e4e7a1 100644 --- a/src/PaymasterHub.sol +++ b/src/PaymasterHub.sol @@ -126,6 +126,7 @@ contract PaymasterHub is IPaymaster, Initializable, UUPSUpgradeable, ReentrancyG address entryPoint; address hats; address poaManager; + address orgRegistrar; // authorized contract that can register orgs (e.g. OrgDeployer) } // keccak256(abi.encode(uint256(keccak256("poa.paymasterhub.main")) - 1)) @@ -335,6 +336,8 @@ contract PaymasterHub is IPaymaster, Initializable, UUPSUpgradeable, ReentrancyG function registerOrgWithVoucher(bytes32 orgId, uint256 adminHatId, uint256 operatorHatId, uint256 voucherHatId) public { + MainStorage storage main = _getMainStorage(); + if (msg.sender != main.poaManager && msg.sender != main.orgRegistrar) revert NotPoaManager(); if (adminHatId == 0) revert ZeroAddress(); mapping(bytes32 => OrgConfig) storage orgs = _getOrgsStorage(); @@ -611,18 +614,17 @@ contract PaymasterHub is IPaymaster, Initializable, UUPSUpgradeable, ReentrancyG mapping(bytes32 => OrgFinancials) storage financials = _getFinancialsStorage(); OrgFinancials storage org = financials[orgId]; - // Calculate total available funds - uint256 totalAvailable = uint256(org.deposited) - uint256(org.spent); + uint256 depositAvailable = + uint256(org.deposited) > uint256(org.spent) ? uint256(org.deposited) - uint256(org.spent) : 0; - // Check if org has enough in deposits to cover this - // Note: solidarity is checked separately in _checkSolidarityAccess - if (org.spent + maxCost > org.deposited) { - // Will need to use solidarity - that's checked elsewhere - // Here we just make sure they haven't overdrawn - if (totalAvailable == 0) { - revert InsufficientOrgBalance(); - } + // If deposits alone cover the cost, no solidarity needed + if (depositAvailable >= maxCost) return; + + // Deposits are fully exhausted — revert early before solidarity check + if (depositAvailable == 0) { + revert InsufficientOrgBalance(); } + // Partial coverage: solidarity will cover the rest (validated by _checkSolidarityAccess) } /** @@ -1033,6 +1035,16 @@ contract PaymasterHub is IPaymaster, Initializable, UUPSUpgradeable, ReentrancyG emit GracePeriodConfigUpdated(_initialGraceDays, _maxSpendDuringGrace, _minDepositRequired); } + /** + * @notice Set the authorized org registrar (e.g. OrgDeployer) + * @dev Only PoaManager can set the registrar + * @param registrar Address authorized to call registerOrg + */ + function setOrgRegistrar(address registrar) external { + if (msg.sender != _getMainStorage().poaManager) revert NotPoaManager(); + _getMainStorage().orgRegistrar = registrar; + } + /** * @notice Ban or unban an org from accessing solidarity fund * @dev Only PoaManager can ban orgs for malicious behavior @@ -1665,13 +1677,10 @@ contract PaymasterHub is IPaymaster, Initializable, UUPSUpgradeable, ReentrancyG } if (tip > 0) { - // Update total paid - bounty.totalPaid += uint144(tip); - - // Attempt payment with gas limit (bool success,) = bundlerOrigin.call{value: tip, gas: 30000}(""); if (success) { + bounty.totalPaid += uint144(tip); emit BountyPaid(userOpHash, bundlerOrigin, tip); } else { emit BountyPayFailed(userOpHash, bundlerOrigin, tip); diff --git a/src/TaskManager.sol b/src/TaskManager.sol index f3e5a33..9d316ec 100644 --- a/src/TaskManager.sol +++ b/src/TaskManager.sol @@ -41,6 +41,7 @@ contract TaskManager is Initializable, ContextUpgradeable { error RequiresApplication(); error NoApplicationRequired(); error InvalidIndex(); + error SelfReviewNotAllowed(); /*──────── Constants ─────*/ bytes4 public constant MODULE_ID = 0x54534b32; // "TSK2" @@ -499,10 +500,19 @@ contract TaskManager is Initializable, ContextUpgradeable { function completeTask(uint256 id) external { Layout storage l = _layout(); - _checkPerm(l._tasks[id].projectId, TaskPerm.REVIEW); + bytes32 pid = l._tasks[id].projectId; + _checkPerm(pid, TaskPerm.REVIEW); Task storage t = _task(l, id); if (t.status != Status.SUBMITTED) revert BadStatus(); + // Self-review: if caller is the claimer, require SELF_REVIEW permission or PM/executor + address sender = _msgSender(); + if (t.claimer == sender && !_isPM(pid, sender)) { + if (!TaskPerm.has(_permMask(sender, pid), TaskPerm.SELF_REVIEW)) { + revert SelfReviewNotAllowed(); + } + } + t.status = Status.COMPLETED; l.token.mint(t.claimer, uint256(t.payout)); diff --git a/src/libs/HybridVotingCore.sol b/src/libs/HybridVotingCore.sol index bd40ec8..783aa61 100644 --- a/src/libs/HybridVotingCore.sol +++ b/src/libs/HybridVotingCore.sol @@ -137,6 +137,8 @@ library HybridVotingCore { function announceWinner(uint256 id) external returns (uint256 winner, bool valid) { HybridVoting.Layout storage l = _layout(); HybridVoting.Proposal storage p = l._proposals[id]; + if (p.executed) revert VotingErrors.AlreadyExecuted(); + p.executed = true; // Check if any votes were cast bool hasVotes = false; @@ -191,14 +193,14 @@ library HybridVotingCore { ); IExecutor.Call[] storage batch = p.batches[winner]; - bool executed = false; + bool didExecute = false; if (valid && batch.length > 0) { // No target validation needed - Executor has onlyExecutor permission on all org contracts // and handles the actual calls. HybridVoting just passes the batch through. l.executor.execute(id, batch); - executed = true; + didExecute = true; emit ProposalExecuted(id, winner, batch.length); } - emit Winner(id, winner, valid, executed, uint64(block.timestamp)); + emit Winner(id, winner, valid, didExecute, uint64(block.timestamp)); } } diff --git a/src/libs/TaskPerm.sol b/src/libs/TaskPerm.sol index cf46583..c692802 100644 --- a/src/libs/TaskPerm.sol +++ b/src/libs/TaskPerm.sol @@ -10,6 +10,7 @@ library TaskPerm { uint8 internal constant CLAIM = 1 << 1; uint8 internal constant REVIEW = 1 << 2; uint8 internal constant ASSIGN = 1 << 3; + uint8 internal constant SELF_REVIEW = 1 << 4; function has(uint8 mask, uint8 flag) internal pure returns (bool) { return mask & flag != 0; diff --git a/src/libs/VotingErrors.sol b/src/libs/VotingErrors.sol index 988c7b6..935b07a 100644 --- a/src/libs/VotingErrors.sol +++ b/src/libs/VotingErrors.sol @@ -29,4 +29,5 @@ library VotingErrors { error InvalidSliceSum(); error TooManyClasses(); error InvalidStrategy(); + error AlreadyExecuted(); } diff --git a/test/DeployerTest.t.sol b/test/DeployerTest.t.sol index 7e1ed7c..ebe87b3 100644 --- a/test/DeployerTest.t.sol +++ b/test/DeployerTest.t.sol @@ -726,6 +726,12 @@ contract DeployerTest is Test, IEligibilityModuleEvents { ); deployer = OrgDeployer(address(new BeaconProxy(deployerBeacon, deployerInit))); + // Authorize OrgDeployer to register orgs on PaymasterHub + vm.stopPrank(); + vm.prank(address(poaManager)); + paymasterHub.setOrgRegistrar(address(deployer)); + vm.startPrank(poaAdmin); + // Debug to verify Deployer initialization console.log("deployer address:", address(deployer)); @@ -3980,4 +3986,35 @@ contract DeployerTest is Test, IEligibilityModuleEvents { address beacon = _getBeaconForType(ModuleTypes.TOGGLE_MODULE_ID); assertEq(SwitchableBeacon(beacon).owner(), setup.exec, "Toggle beacon should be owned by executor"); } + + function testVouchRevocationCrossDay() public { + TestOrgSetup memory setup = _createTestOrg("Cross-Day Revoke DAO"); + address candidate = address(0x500); + + _setupUserForVouching(setup.eligibilityModule, setup.exec, voter1); + _setupUserForVouching(setup.eligibilityModule, setup.exec, voter2); + _setupUserForVouching(setup.eligibilityModule, setup.exec, candidate); + + _configureVouching( + setup.eligibilityModule, setup.exec, setup.defaultRoleHat, 2, setup.memberRoleHat, false, true + ); + + _mintHat(setup.exec, setup.memberRoleHat, voter1); + _mintHat(setup.exec, setup.memberRoleHat, voter2); + + // Vouch on day 1 + _vouchFor(voter1, setup.eligibilityModule, candidate, setup.defaultRoleHat); + _vouchFor(voter2, setup.eligibilityModule, candidate, setup.defaultRoleHat); + + // Warp to a different day (previously caused underflow in dailyVouchCount decrement) + vm.warp(block.timestamp + 2 days); + + // Revoke on day 3 — should succeed without underflow + _revokeVouch(voter1, setup.eligibilityModule, candidate, setup.defaultRoleHat); + + // Verify the revocation worked correctly + _assertVouchStatus( + setup.eligibilityModule, candidate, setup.defaultRoleHat, 1, false, "After cross-day revocation" + ); + } } diff --git a/test/DirectDemocracyVoting.t.sol b/test/DirectDemocracyVoting.t.sol index 7c2d00e..1ef2528 100644 --- a/test/DirectDemocracyVoting.t.sol +++ b/test/DirectDemocracyVoting.t.sol @@ -612,4 +612,32 @@ contract DDVotingTest is Test { assertEq(hats.balanceOf(bob, executiveHatId), 0, "Bob should not have executive hat"); assertEq(hats.balanceOf(bob, managerHatId), 0, "Bob should not have manager hat"); } + + /*//////////////////////////////////////////////////////////// + ANNOUNCE WINNER REPLAY PROTECTION + ////////////////////////////////////////////////////////////*/ + + function testAnnounceWinnerDoubleCallReverts() public { + IExecutor.Call[][] memory b = new IExecutor.Call[][](2); + b[0] = new IExecutor.Call[](0); + b[1] = new IExecutor.Call[](0); + vm.prank(creator); + dd.createProposal(bytes("Replay Test"), bytes32(0), 10, 2, b, new uint256[](0)); + + uint8[] memory idx = new uint8[](1); + idx[0] = 0; + uint8[] memory w = new uint8[](1); + w[0] = 100; + vm.prank(voter); + dd.vote(0, idx, w); + + vm.warp(block.timestamp + 11 minutes); + + // First call succeeds + dd.announceWinner(0); + + // Second call reverts + vm.expectRevert(VotingErrors.AlreadyExecuted.selector); + dd.announceWinner(0); + } } diff --git a/test/Executor.t.sol b/test/Executor.t.sol index 18c19e7..c90a6e3 100644 --- a/test/Executor.t.sol +++ b/test/Executor.t.sol @@ -70,4 +70,23 @@ contract ExecutorTest is Test { vm.expectRevert(Executor.UnauthorizedCaller.selector); exec.mintHatsForUser(user, hatIds); } + + function testSetCallerUnauthorizedReverts() public { + address random = address(0x99); + vm.prank(random); + vm.expectRevert(Executor.UnauthorizedCaller.selector); + exec.setCaller(address(0x5)); + } + + function testSetCallerZeroAddressReverts() public { + vm.expectRevert(Executor.ZeroAddress.selector); + exec.setCaller(address(0)); + } + + function testAllowedCallerCanSetNewCaller() public { + address newCaller = address(0x5); + vm.prank(caller); + exec.setCaller(newCaller); + assertEq(exec.allowedCaller(), newCaller); + } } diff --git a/test/HybridVoting.t.sol b/test/HybridVoting.t.sol index c3ea412..dc128d5 100644 --- a/test/HybridVoting.t.sol +++ b/test/HybridVoting.t.sol @@ -1483,4 +1483,24 @@ contract MockERC20 is IERC20 { assertTrue(ok, "Should have quorum with multiple participants"); // The result depends on the complex interaction of all classes } + + /* ───────────── ANNOUNCE WINNER REPLAY PROTECTION ───────────── */ + + function testAnnounceWinnerDoubleCallReverts() public { + uint256 id = _create(); + + _voteYES(alice); + _voteYES(carol); + + vm.warp(block.timestamp + 16 minutes); + + // First call succeeds + vm.prank(alice); + hv.announceWinner(id); + + // Second call reverts + vm.expectRevert(VotingErrors.AlreadyExecuted.selector); + vm.prank(alice); + hv.announceWinner(id); + } } diff --git a/test/Passkey.t.sol b/test/Passkey.t.sol index 222b833..10a7f86 100644 --- a/test/Passkey.t.sol +++ b/test/Passkey.t.sol @@ -1061,4 +1061,66 @@ contract PasskeyTest is Test { assertFalse(P256Verifier.isValidSignature(bytes32(0), s)); assertFalse(P256Verifier.isValidSignature(r, bytes32(0))); } + + /*════════════════════════════════════════════════════════════════════ + ZERO PUBKEY VALIDATION TESTS + ════════════════════════════════════════════════════════════════════*/ + + function testAddCredentialZeroPubKeyXReverts() public { + PasskeyAccount account = _createAccount(); + + vm.prank(address(account)); + vm.expectRevert(IPasskeyAccount.InvalidSignature.selector); + account.addCredential(CREDENTIAL_ID_2, bytes32(0), PUB_KEY_Y); + } + + function testAddCredentialZeroPubKeyYReverts() public { + PasskeyAccount account = _createAccount(); + + vm.prank(address(account)); + vm.expectRevert(IPasskeyAccount.InvalidSignature.selector); + account.addCredential(CREDENTIAL_ID_2, PUB_KEY_X, bytes32(0)); + } + + function testInitiateRecoveryZeroPubKeyXReverts() public { + PasskeyAccount account = _createAccount(); + + vm.prank(guardian); + vm.expectRevert(IPasskeyAccount.InvalidSignature.selector); + account.initiateRecovery(keccak256("new_cred"), bytes32(0), PUB_KEY_Y); + } + + function testInitiateRecoveryZeroPubKeyYReverts() public { + PasskeyAccount account = _createAccount(); + + vm.prank(guardian); + vm.expectRevert(IPasskeyAccount.InvalidSignature.selector); + account.initiateRecovery(keccak256("new_cred"), PUB_KEY_X, bytes32(0)); + } + + function testCompleteRecoveryMaxCredentialsReverts() public { + // Set max credentials to 2 + vm.prank(owner); + factory.setMaxCredentials(2); + + PasskeyAccount account = _createAccount(); + + // Add second credential to reach the limit + vm.prank(address(account)); + account.addCredential(CREDENTIAL_ID_2, PUB_KEY_X, PUB_KEY_Y); + + // Initiate recovery (would add a third credential) + bytes32 recoveryCredId = keccak256("recovery_cred"); + vm.prank(guardian); + account.initiateRecovery(recoveryCredId, keccak256("rx"), keccak256("ry")); + + bytes32 recoveryId = keccak256(abi.encodePacked(recoveryCredId, block.timestamp, guardian)); + + // Warp past recovery delay + vm.warp(block.timestamp + 7 days + 1); + + // Complete recovery should revert — already at max credentials + vm.expectRevert(IPasskeyAccount.MaxCredentialsReached.selector); + account.completeRecovery(recoveryId); + } } diff --git a/test/PasskeyPaymasterIntegration.t.sol b/test/PasskeyPaymasterIntegration.t.sol index 73c6932..6a4ac84 100644 --- a/test/PasskeyPaymasterIntegration.t.sol +++ b/test/PasskeyPaymasterIntegration.t.sol @@ -153,11 +153,12 @@ contract PasskeyPaymasterIntegrationTest is Test { ERC1967Proxy hubProxy = new ERC1967Proxy(address(hubImpl), hubInitData); hub = PaymasterHub(payable(address(hubProxy))); - // Register org in PaymasterHub with voucher hat - hub.registerOrgWithVoucher(ORG_ID, ADMIN_HAT, OPERATOR_HAT, VOUCHER_HAT); - vm.stopPrank(); + // Register org in PaymasterHub with voucher hat (requires poaManager) + vm.prank(poaManager); + hub.registerOrgWithVoucher(ORG_ID, ADMIN_HAT, OPERATOR_HAT, VOUCHER_HAT); + // Fund test accounts BEFORE using them for deposits vm.deal(owner, 100 ether); vm.deal(orgAdmin, 100 ether); @@ -929,7 +930,7 @@ contract PasskeyPaymasterIntegrationTest is Test { // Create a new org WITHOUT voucher hat bytes32 noVoucherOrgId = keccak256("NO_VOUCHER_ORG"); - vm.startPrank(owner); + vm.startPrank(poaManager); hub.registerOrg(noVoucherOrgId, ADMIN_HAT, OPERATOR_HAT); // No voucher hat! vm.stopPrank(); diff --git a/test/PaymasterHubSolidarity.t.sol b/test/PaymasterHubSolidarity.t.sol index 7506e92..6342ba5 100644 --- a/test/PaymasterHubSolidarity.t.sol +++ b/test/PaymasterHubSolidarity.t.sol @@ -287,10 +287,12 @@ contract PaymasterHubSolidarityTest is Test { vm.deal(user1, 100 ether); vm.deal(user2, 100 ether); - // Register orgs + // Register orgs (requires poaManager) + vm.startPrank(poaManager); hub.registerOrg(ORG_ALPHA, ADMIN_HAT, OPERATOR_HAT); hub.registerOrg(ORG_BETA, ADMIN_HAT, OPERATOR_HAT); hub.registerOrg(ORG_GAMMA, ADMIN_HAT, OPERATOR_HAT); + vm.stopPrank(); } // ============ Initialization Tests ============ @@ -316,9 +318,11 @@ contract PaymasterHubSolidarityTest is Test { function testOrgRegistration() public { bytes32 newOrgId = keccak256("NEW_ORG"); + vm.startPrank(poaManager); vm.expectEmit(true, false, false, true); emit OrgRegistered(newOrgId, ADMIN_HAT, OPERATOR_HAT); hub.registerOrg(newOrgId, ADMIN_HAT, OPERATOR_HAT); + vm.stopPrank(); PaymasterHub.OrgConfig memory config = hub.getOrgConfig(newOrgId); assertEq(config.adminHatId, ADMIN_HAT); @@ -931,4 +935,43 @@ contract PaymasterHubSolidarityTest is Test { assertFalse(requiresDeposit); assertEq(solidarityLimit, 0.006 ether); // 2x match } + + // ============ registerOrg Access Control Tests ============ + + function testRegisterOrgUnauthorizedReverts() public { + bytes32 newOrgId = keccak256("UNAUTHORIZED_ORG"); + + vm.prank(orgAdmin); + vm.expectRevert(PaymasterHub.NotPoaManager.selector); + hub.registerOrg(newOrgId, ADMIN_HAT, OPERATOR_HAT); + } + + function testRegisterOrgRandomUserReverts() public { + bytes32 newOrgId = keccak256("RANDOM_ORG"); + + vm.prank(user1); + vm.expectRevert(PaymasterHub.NotPoaManager.selector); + hub.registerOrg(newOrgId, ADMIN_HAT, OPERATOR_HAT); + } + + function testSetOrgRegistrar() public { + address registrar = address(0x99); + + vm.prank(poaManager); + hub.setOrgRegistrar(registrar); + + // Registrar can now register orgs + bytes32 newOrgId = keccak256("REGISTRAR_ORG"); + vm.prank(registrar); + hub.registerOrg(newOrgId, ADMIN_HAT, OPERATOR_HAT); + + PaymasterHub.OrgConfig memory config = hub.getOrgConfig(newOrgId); + assertEq(config.adminHatId, ADMIN_HAT); + } + + function testSetOrgRegistrarUnauthorizedReverts() public { + vm.prank(orgAdmin); + vm.expectRevert(PaymasterHub.NotPoaManager.selector); + hub.setOrgRegistrar(address(0x99)); + } } diff --git a/test/TaskManager.t.sol b/test/TaskManager.t.sol index 6b75fff..44c34de 100644 --- a/test/TaskManager.t.sol +++ b/test/TaskManager.t.sol @@ -5898,3 +5898,99 @@ contract MockToken is Test, IERC20 { tm.bootstrapProjectsAndTasks(moreProjects, moreTasks); } } + + /*──────────────────── Self-Review Tests ────────────────────*/ + contract TaskManagerSelfReviewTest is TaskManagerTestBase { + uint256 constant REVIEWER_HAT = 4; + address reviewer = makeAddr("reviewer"); + bytes32 projectId; + + function setUp() public { + setUpBase(); + setHat(reviewer, REVIEWER_HAT); + // CLAIM | REVIEW but NOT SELF_REVIEW + vm.prank(executor); + tm.setConfig( + TaskManager.ConfigKey.ROLE_PERM, abi.encode(REVIEWER_HAT, TaskPerm.CLAIM | TaskPerm.REVIEW) + ); + + projectId = _createDefaultProject("SELF_REVIEW", 10 ether); + } + + function test_SelfReviewBlockedWithoutPermission() public { + vm.prank(creator1); + tm.createTask(1 ether, bytes("task"), bytes32(0), projectId, address(0), 0, false); + + vm.prank(reviewer); + tm.claimTask(0); + + vm.prank(reviewer); + tm.submitTask(0, keccak256("work")); + + vm.prank(reviewer); + vm.expectRevert(TaskManager.SelfReviewNotAllowed.selector); + tm.completeTask(0); + } + + function test_SelfReviewAllowedWithPermission() public { + // Grant SELF_REVIEW in addition to CLAIM | REVIEW + vm.prank(executor); + tm.setConfig( + TaskManager.ConfigKey.ROLE_PERM, + abi.encode(REVIEWER_HAT, TaskPerm.CLAIM | TaskPerm.REVIEW | TaskPerm.SELF_REVIEW) + ); + + vm.prank(creator1); + tm.createTask(1 ether, bytes("task"), bytes32(0), projectId, address(0), 0, false); + + vm.prank(reviewer); + tm.claimTask(0); + + vm.prank(reviewer); + tm.submitTask(0, keccak256("work")); + + vm.prank(reviewer); + tm.completeTask(0); + + assertEq(token.balanceOf(reviewer), 1 ether); + } + + function test_PMCanAlwaysReviewOwnTask() public { + vm.prank(executor); + tm.setConfig(TaskManager.ConfigKey.PROJECT_MANAGER, abi.encode(projectId, pm1, true)); + + vm.prank(pm1); + tm.createTask(1 ether, bytes("pm_task"), bytes32(0), projectId, address(0), 0, false); + + // PM bypasses _checkPerm, so can claim even without CLAIM flag + vm.prank(pm1); + tm.claimTask(0); + + vm.prank(pm1); + tm.submitTask(0, keccak256("pm_work")); + + // PM bypasses self-review check + vm.prank(pm1); + tm.completeTask(0); + + assertEq(token.balanceOf(pm1), 1 ether); + } + + function test_DifferentReviewerCanAlwaysComplete() public { + vm.prank(creator1); + tm.createTask(1 ether, bytes("task"), bytes32(0), projectId, address(0), 0, false); + + // reviewer claims and submits + vm.prank(reviewer); + tm.claimTask(0); + + vm.prank(reviewer); + tm.submitTask(0, keccak256("work")); + + // pm1 (different person with REVIEW permission) completes — always allowed + vm.prank(pm1); + tm.completeTask(0); + + assertEq(token.balanceOf(reviewer), 1 ether); + } + }