Post-audit phase 9 (rebased): tightened test asserts for kToken0 - #12
Merged
Conversation
Tighten every `vm.expectRevert()` (bare) across kToken / kOFT / kOFTAdapter unit tests to use specific selectors so role-check or state-check regressions cannot pass silently behind unrelated reverts (insufficient balance, allowance, proxy initialization, etc.). Tightened reverts (44 total): - kToken.t.sol (29) — KTOKEN_WRONG_ROLE / KTOKEN_IS_PAUSED / KTOKEN_ACCOUNT_FROZEN / KTOKEN_ZERO_ADDRESS, ERC20.InsufficientBalance, ERC20.InsufficientAllowance, Ownable.Unauthorized, Initializable.InvalidInitialization - kOFT.t.sol (11) — same plus kOFT.ZeroAddress and OZ's OwnableUnauthorizedAccount(address) for OFTCoreUpgradeable's owner check - kOFTAdater.t.sol (4) — KTOKEN_WRONG_ROLE, Initializable.InvalidInitialization Added missing tests (17 in kToken.t.sol): - Role revocation: revokeAdminRole (success / non-owner / revoked admin cannot grant minter), revokeEmergencyRole (success / non-admin / revoked emergency admin cannot pause), grantEmergencyRole non-admin revert. - emergencyWithdraw input validation: zero destination, zero amount, ETH transfer failure (via a non-payable rejector contract). - initialize zero-address checks: zero owner / admin / emergency admin / minter — each spun up via MinimalUUPSFactory so the revert lands during the initializer call. - UUPS upgrade authorization: success (verifies the ERC-1967 implementation slot), non-owner revert, zero-implementation revert. Mutation-verified: 12 strategic source mutations on kToken.sol — all 12 killed by the suite (skip role checks in mint / grantMinterRole / freeze / setPaused / emergencyWithdraw, skip _checkOwner in _authorizeUpgrade, skip _checkPaused / _checkNotFrozen in _beforeTokenTransfer, weaken the owner / zero-amount / zero-address / zero-owner-init guards). Suite: 120 -> 137 passing, no regressions.
fepvenancio
added a commit
that referenced
this pull request
May 19, 2026
* Factory tests (#8) * fix: unecessary _checkPaused on crosschain functions * added: MinimalUUPSProxyFactory * fix: new factory dependency * fix: reviews * fix: unecessary _checkPaused on crosschain functions (#7) * fix: [High] frozen users can renounce role * fix: split LayerZero delegate from contract owner in kOFT and kOFTAdapter kOFT.initialize and kOFTAdapter.initialize now take two distinct addresses — the LayerZero delegate (send/receive libs, DVN, executor config) and the contract owner (upgrade authority, setDelegate authority). Collapsing both into a single key removed a defense-in-depth boundary LayerZero exposes by design. Adds zero-address checks on both initializers; kOFTAdapter previously declared no ZeroAddress error, now mirrors kOFT. Deployment config schema extended: RoleAddresses now has a `delegate` field; validateConfig requires it non-zero; all 12 network JSONs get a `delegate` placeholder. Operators MAY collapse delegate and owner onto the same multisig by setting both fields to the same address. Tests: 8 new tests (4 per contract) lock in the distinct-address semantics — zero-delegate revert, zero-owner revert, distinct-address success, non-owner cannot rotate delegate via setDelegate. Docs: architecture.md and README.md updated to reflect the new initialize signature and role hierarchy. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Post audit phase 4 (#9) * refactor(ERC3009): adopt ERC-7201 namespaced storage The `_authorizationStates` mapping lived as an unstructured base storage slot, leaving any contract inheriting from `ERC3009` (currently `kToken`) exposed to slot collisions on subsequent storage additions to either side of the inheritance hierarchy. Wrapped the mapping in an `ERC3009Storage` struct keyed at the standard ERC-7201 location `erc7201:kam.storage.ERC3009`, accessed via `_getERC3009Storage()`. This is a storage-layout change. Pre-deployment so no migration concern, but any future deployment must run from a clean slot — existing slot-0 authorization data (if any was set in tests) is no longer reachable through the new accessor. Phase 4 item 15 of 19. 129 / 129 tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(kToken): use KTOKEN_CANNOT_FREEZE_OWNER for owner-freeze attempts `freeze(owner)` reverted with `KTOKEN_WRONG_ROLE` ("T5"), which is the generic role-check error used everywhere. The condition has nothing to do with the caller's role — the caller already passed `_checkBlacklistAdmin(msg.sender)` — it's a constraint on the *target* address: the owner cannot be blacklisted. Added `KTOKEN_CANNOT_FREEZE_OWNER = "T7"` and pointed the require at it. Phase 4 item 17 of 19. 129 / 129 tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> * Post-audit phase 9 (rebased): tightened test asserts for kToken0 (#12) * test: tighten 44 bare expectReverts and add 17 tests (Phase 9) Tighten every `vm.expectRevert()` (bare) across kToken / kOFT / kOFTAdapter unit tests to use specific selectors so role-check or state-check regressions cannot pass silently behind unrelated reverts (insufficient balance, allowance, proxy initialization, etc.). Tightened reverts (44 total): - kToken.t.sol (29) — KTOKEN_WRONG_ROLE / KTOKEN_IS_PAUSED / KTOKEN_ACCOUNT_FROZEN / KTOKEN_ZERO_ADDRESS, ERC20.InsufficientBalance, ERC20.InsufficientAllowance, Ownable.Unauthorized, Initializable.InvalidInitialization - kOFT.t.sol (11) — same plus kOFT.ZeroAddress and OZ's OwnableUnauthorizedAccount(address) for OFTCoreUpgradeable's owner check - kOFTAdater.t.sol (4) — KTOKEN_WRONG_ROLE, Initializable.InvalidInitialization Added missing tests (17 in kToken.t.sol): - Role revocation: revokeAdminRole (success / non-owner / revoked admin cannot grant minter), revokeEmergencyRole (success / non-admin / revoked emergency admin cannot pause), grantEmergencyRole non-admin revert. - emergencyWithdraw input validation: zero destination, zero amount, ETH transfer failure (via a non-payable rejector contract). - initialize zero-address checks: zero owner / admin / emergency admin / minter — each spun up via MinimalUUPSFactory so the revert lands during the initializer call. - UUPS upgrade authorization: success (verifies the ERC-1967 implementation slot), non-owner revert, zero-implementation revert. Mutation-verified: 12 strategic source mutations on kToken.sol — all 12 killed by the suite (skip role checks in mint / grantMinterRole / freeze / setPaused / emergencyWithdraw, skip _checkOwner in _authorizeUpgrade, skip _checkPaused / _checkNotFrozen in _beforeTokenTransfer, weaken the owner / zero-amount / zero-address / zero-owner-init guards). Suite: 120 -> 137 passing, no regressions. * chore: forge fmt (1.7.0) * Docs updated * chore: gitignore soldeer.lock and pin minimal-uups-factory dependency Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * forge fmt --------- Co-authored-by: addressZero <filipeepv@gmail.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Solthodox
added a commit
that referenced
this pull request
May 19, 2026
* fix: unecessary _checkPaused on crosschain functions * Audit fixes tob (#13) * Factory tests (#8) * fix: unecessary _checkPaused on crosschain functions * added: MinimalUUPSProxyFactory * fix: new factory dependency * fix: reviews * fix: unecessary _checkPaused on crosschain functions (#7) * fix: [High] frozen users can renounce role * fix: split LayerZero delegate from contract owner in kOFT and kOFTAdapter kOFT.initialize and kOFTAdapter.initialize now take two distinct addresses — the LayerZero delegate (send/receive libs, DVN, executor config) and the contract owner (upgrade authority, setDelegate authority). Collapsing both into a single key removed a defense-in-depth boundary LayerZero exposes by design. Adds zero-address checks on both initializers; kOFTAdapter previously declared no ZeroAddress error, now mirrors kOFT. Deployment config schema extended: RoleAddresses now has a `delegate` field; validateConfig requires it non-zero; all 12 network JSONs get a `delegate` placeholder. Operators MAY collapse delegate and owner onto the same multisig by setting both fields to the same address. Tests: 8 new tests (4 per contract) lock in the distinct-address semantics — zero-delegate revert, zero-owner revert, distinct-address success, non-owner cannot rotate delegate via setDelegate. Docs: architecture.md and README.md updated to reflect the new initialize signature and role hierarchy. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Post audit phase 4 (#9) * refactor(ERC3009): adopt ERC-7201 namespaced storage The `_authorizationStates` mapping lived as an unstructured base storage slot, leaving any contract inheriting from `ERC3009` (currently `kToken`) exposed to slot collisions on subsequent storage additions to either side of the inheritance hierarchy. Wrapped the mapping in an `ERC3009Storage` struct keyed at the standard ERC-7201 location `erc7201:kam.storage.ERC3009`, accessed via `_getERC3009Storage()`. This is a storage-layout change. Pre-deployment so no migration concern, but any future deployment must run from a clean slot — existing slot-0 authorization data (if any was set in tests) is no longer reachable through the new accessor. Phase 4 item 15 of 19. 129 / 129 tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(kToken): use KTOKEN_CANNOT_FREEZE_OWNER for owner-freeze attempts `freeze(owner)` reverted with `KTOKEN_WRONG_ROLE` ("T5"), which is the generic role-check error used everywhere. The condition has nothing to do with the caller's role — the caller already passed `_checkBlacklistAdmin(msg.sender)` — it's a constraint on the *target* address: the owner cannot be blacklisted. Added `KTOKEN_CANNOT_FREEZE_OWNER = "T7"` and pointed the require at it. Phase 4 item 17 of 19. 129 / 129 tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> * Post-audit phase 9 (rebased): tightened test asserts for kToken0 (#12) * test: tighten 44 bare expectReverts and add 17 tests (Phase 9) Tighten every `vm.expectRevert()` (bare) across kToken / kOFT / kOFTAdapter unit tests to use specific selectors so role-check or state-check regressions cannot pass silently behind unrelated reverts (insufficient balance, allowance, proxy initialization, etc.). Tightened reverts (44 total): - kToken.t.sol (29) — KTOKEN_WRONG_ROLE / KTOKEN_IS_PAUSED / KTOKEN_ACCOUNT_FROZEN / KTOKEN_ZERO_ADDRESS, ERC20.InsufficientBalance, ERC20.InsufficientAllowance, Ownable.Unauthorized, Initializable.InvalidInitialization - kOFT.t.sol (11) — same plus kOFT.ZeroAddress and OZ's OwnableUnauthorizedAccount(address) for OFTCoreUpgradeable's owner check - kOFTAdater.t.sol (4) — KTOKEN_WRONG_ROLE, Initializable.InvalidInitialization Added missing tests (17 in kToken.t.sol): - Role revocation: revokeAdminRole (success / non-owner / revoked admin cannot grant minter), revokeEmergencyRole (success / non-admin / revoked emergency admin cannot pause), grantEmergencyRole non-admin revert. - emergencyWithdraw input validation: zero destination, zero amount, ETH transfer failure (via a non-payable rejector contract). - initialize zero-address checks: zero owner / admin / emergency admin / minter — each spun up via MinimalUUPSFactory so the revert lands during the initializer call. - UUPS upgrade authorization: success (verifies the ERC-1967 implementation slot), non-owner revert, zero-implementation revert. Mutation-verified: 12 strategic source mutations on kToken.sol — all 12 killed by the suite (skip role checks in mint / grantMinterRole / freeze / setPaused / emergencyWithdraw, skip _checkOwner in _authorizeUpgrade, skip _checkPaused / _checkNotFrozen in _beforeTokenTransfer, weaken the owner / zero-amount / zero-address / zero-owner-init guards). Suite: 120 -> 137 passing, no regressions. * chore: forge fmt (1.7.0) * Docs updated * chore: gitignore soldeer.lock and pin minimal-uups-factory dependency Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * forge fmt --------- Co-authored-by: addressZero <filipeepv@gmail.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * Delete soldeer.lock * forge fmt and asserted merge --------- Co-authored-by: Solthodox <100046219+Solthodox@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Solthodox
added a commit
that referenced
this pull request
May 19, 2026
* fix: unecessary _checkPaused on crosschain functions * Audit fixes tob (#13) * Factory tests (#8) * fix: unecessary _checkPaused on crosschain functions * added: MinimalUUPSProxyFactory * fix: new factory dependency * fix: reviews * fix: unecessary _checkPaused on crosschain functions (#7) * fix: [High] frozen users can renounce role * fix: split LayerZero delegate from contract owner in kOFT and kOFTAdapter kOFT.initialize and kOFTAdapter.initialize now take two distinct addresses — the LayerZero delegate (send/receive libs, DVN, executor config) and the contract owner (upgrade authority, setDelegate authority). Collapsing both into a single key removed a defense-in-depth boundary LayerZero exposes by design. Adds zero-address checks on both initializers; kOFTAdapter previously declared no ZeroAddress error, now mirrors kOFT. Deployment config schema extended: RoleAddresses now has a `delegate` field; validateConfig requires it non-zero; all 12 network JSONs get a `delegate` placeholder. Operators MAY collapse delegate and owner onto the same multisig by setting both fields to the same address. Tests: 8 new tests (4 per contract) lock in the distinct-address semantics — zero-delegate revert, zero-owner revert, distinct-address success, non-owner cannot rotate delegate via setDelegate. Docs: architecture.md and README.md updated to reflect the new initialize signature and role hierarchy. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Post audit phase 4 (#9) * refactor(ERC3009): adopt ERC-7201 namespaced storage The `_authorizationStates` mapping lived as an unstructured base storage slot, leaving any contract inheriting from `ERC3009` (currently `kToken`) exposed to slot collisions on subsequent storage additions to either side of the inheritance hierarchy. Wrapped the mapping in an `ERC3009Storage` struct keyed at the standard ERC-7201 location `erc7201:kam.storage.ERC3009`, accessed via `_getERC3009Storage()`. This is a storage-layout change. Pre-deployment so no migration concern, but any future deployment must run from a clean slot — existing slot-0 authorization data (if any was set in tests) is no longer reachable through the new accessor. Phase 4 item 15 of 19. 129 / 129 tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(kToken): use KTOKEN_CANNOT_FREEZE_OWNER for owner-freeze attempts `freeze(owner)` reverted with `KTOKEN_WRONG_ROLE` ("T5"), which is the generic role-check error used everywhere. The condition has nothing to do with the caller's role — the caller already passed `_checkBlacklistAdmin(msg.sender)` — it's a constraint on the *target* address: the owner cannot be blacklisted. Added `KTOKEN_CANNOT_FREEZE_OWNER = "T7"` and pointed the require at it. Phase 4 item 17 of 19. 129 / 129 tests pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> * Post-audit phase 9 (rebased): tightened test asserts for kToken0 (#12) * test: tighten 44 bare expectReverts and add 17 tests (Phase 9) Tighten every `vm.expectRevert()` (bare) across kToken / kOFT / kOFTAdapter unit tests to use specific selectors so role-check or state-check regressions cannot pass silently behind unrelated reverts (insufficient balance, allowance, proxy initialization, etc.). Tightened reverts (44 total): - kToken.t.sol (29) — KTOKEN_WRONG_ROLE / KTOKEN_IS_PAUSED / KTOKEN_ACCOUNT_FROZEN / KTOKEN_ZERO_ADDRESS, ERC20.InsufficientBalance, ERC20.InsufficientAllowance, Ownable.Unauthorized, Initializable.InvalidInitialization - kOFT.t.sol (11) — same plus kOFT.ZeroAddress and OZ's OwnableUnauthorizedAccount(address) for OFTCoreUpgradeable's owner check - kOFTAdater.t.sol (4) — KTOKEN_WRONG_ROLE, Initializable.InvalidInitialization Added missing tests (17 in kToken.t.sol): - Role revocation: revokeAdminRole (success / non-owner / revoked admin cannot grant minter), revokeEmergencyRole (success / non-admin / revoked emergency admin cannot pause), grantEmergencyRole non-admin revert. - emergencyWithdraw input validation: zero destination, zero amount, ETH transfer failure (via a non-payable rejector contract). - initialize zero-address checks: zero owner / admin / emergency admin / minter — each spun up via MinimalUUPSFactory so the revert lands during the initializer call. - UUPS upgrade authorization: success (verifies the ERC-1967 implementation slot), non-owner revert, zero-implementation revert. Mutation-verified: 12 strategic source mutations on kToken.sol — all 12 killed by the suite (skip role checks in mint / grantMinterRole / freeze / setPaused / emergencyWithdraw, skip _checkOwner in _authorizeUpgrade, skip _checkPaused / _checkNotFrozen in _beforeTokenTransfer, weaken the owner / zero-amount / zero-address / zero-owner-init guards). Suite: 120 -> 137 passing, no regressions. * chore: forge fmt (1.7.0) * Docs updated * chore: gitignore soldeer.lock and pin minimal-uups-factory dependency Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * forge fmt --------- Co-authored-by: addressZero <filipeepv@gmail.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * Delete soldeer.lock * forge fmt and asserted merge * chore: align Solidity 0.8.34 and sync vendor with KAM (#15) --------- Co-authored-by: Solthodox <100046219+Solthodox@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebase of
post-audit-phase-9ontoaudit-fixes-tob.post-audit-phase-9was branched off pre-audit-fix history and conflicted with the audit fixes for findings #11 (frozen-account self-unfreeze) and #19 (LayerZero delegate split). This branch resolves those conflicts and aligns the new tests with the audit-fixed source.Changes brought from phase-9
expectRevert()calls acrosskToken.t.sol,kOFT.t.sol,kOFTAdater.t.solto assert specific selectors / error stringsConflict resolutions
test/unit/kOFT.t.sol,test/unit/kOFTAdater.t.sol: kept phase-9's tightenedInitializable.InvalidInitialization.selectorrevert + audit-fixes-tob's new 2-arginitialize(delegate, owner)signaturetest/unit/kToken.t.sol:test_CannotFreezeOwnernow expectsKTOKEN_CANNOT_FREEZE_OWNER(audit-fixes-tob renamed the error fromKTOKEN_WRONG_ROLEwhen finding test: tighten 44 bare expectReverts + 17 new tests (Phase 9) #11 was fixed)Validation
forge clean && forge build— ✅forge test— ✅ 146/146 passing