review(ens): pre-draft calldata verification for kpk Endowment permissions Update #10 - #102
review(ens): pre-draft calldata verification for kpk Endowment permissions Update #10#102alextnetto wants to merge 14 commits into
Conversation
3df746f to
74faa82
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3df746fa89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _assertTargetNotAllowed(SYRUP_USDC, _approveCall(GPV2_VAULT_RELAYER)); | ||
| _assertTargetNotAllowed(SYRUP_USDT, _approveCall(GPV2_VAULT_RELAYER)); |
There was a problem hiding this comment.
If the finalized payload implements item 5 by adding syrupUSDC and syrupUSDT to the existing ICowSwapOrderSigner.signOrder sell/buy-token conditions, these calls will still target the syrup token contracts themselves and remain TargetAddressNotAllowed. Consequently, the assertions will keep passing even though the documented gap has been fixed, contrary to the stated expectation that the test should flag the update. Exercise signOrder with each syrup token as the sell and buy token instead; token approval permissions are independent of the CoW token lists.
Useful? React with 👍 / 👎.
| Unlike #14, **#15 is still Queued** (`state(15) = Queued`); the node shows its pools still point at their old | ||
| controllers, so this batch has **not** taken effect yet — there is still a window to revoke before it can execute. ✅ |
There was a problem hiding this comment.
For anyone relying on this report after the reviewed commit date, this promise of a remaining pre-execution window is already inconsistent with the report's own timeline: proposal #15 was created on July 9, and §5 says its createTime → eta interval is exactly seven days, while this commit is dated August 1. The block-25,535,332 snapshot can therefore no longer establish that the proposal is still pre-executable; by publication it must have advanced to Grace, Executed, or Expired. Refresh the state at publication time and timestamp the result instead of telling exposed users that they still have time before execution.
Useful? React with 👍 / 👎.
| We reconstructed all 54 transactions of the payload from the specification and from the public contract interfaces, and | ||
| compared our reconstruction against the published payload byte for byte. The two are identical. The permissions granted |
There was a problem hiding this comment.
The claim that all 54 transactions were reconstructed from the specification and public interfaces cannot hold for this payload: the same post says transactions 0–7 deploy and configure a sub-Roles module that the specification does not mention, and the README says transaction 53's annotation content was extracted from the payload; the unmentioned approval-list extensions likewise are not derivable from the specification. The byte comparison still validates the hand encoding, but presenting it as a wholly specification-derived reconstruction overstates its independence. Identify which fields came from the executable payload or configuration repository and reserve the specification-derived claim for the transactions it actually describes.
Useful? React with 👍 / 👎.
1 participant
Summary
Pre-draft calldata security review of Endowment permissions to KPK Update #10.
There is no Tally draft yet. The reviewed artifact is the Safe Transaction Builder payload published with the forum post:
karpatkey/client-configs@ens-dao-manager-harvest-rwa-yield, commit3e447f8d21(clients/ens-dao/mainnet/payloads/ensPermissionsUpdate10.json, 54 transactions).What the test does
_generateCallData()manually derives the full 54-transaction MultiSend body from the forum specification, the payload, and Solidity interfaces (no hex blobs, every selector from an interface, every address a named constant)_assertDerivedPayloadMatches()diffs the derivation byte for byte againstexpectedMultiSend.txt, the published payload re-encoded verbatim (verified with a negative control: a one-byte change fails the test)_beforeProposal()proves the new venues are unreachable today and that the three reward distributors are already claimable with payouts pinned to the Safe_afterExecution()asserts every new permission positively and negatively: vault deposit/withdraw/redeem pinned to the Safe, Aave Horizon pinned to RLUSD, Pendle pinned to the PT-sUSDS-26NOV2026 market with the external-router escape hatch shut, approve() spenders pinned, 19 pre-existing approvals preserved, andsignOrderprobes proving the syrup tokens are not on the CoW lists_assertAssumedHarvestArchitecture()simulates the working assumption for the sub-Roles module: kpk configures the Harvest role on it post-execution (operator0x14C2…97de, per their config repo). Verified: claims pass with payouts to the Safe; redirected payouts are blocked by the main MANAGER layer even with the sub-role configured wide open; the operator can reach nothing beyond the three distributorsVerification result
Derived calldata matches the published payload exactly. Permission shapes are tight. No permissions removed (confirmed by simulation and by the additions-only diff of
roles/MANAGERin karpatkey's config repo).Findings (see forum-post.md for the questions put to kpk)
Recommendation: NEEDS_REVIEW. Questions for kpk are drafted in
forum-post.md.Test
1 passed. Will be re-run against the final payload when the Tally draft is published.