Reject an EIP-7702 delegated account as a clone implementation - #87
thedavidmeister wants to merge 2 commits into
Conversation
`checkImplementationCode` only tested `EXTCODESIZE != 0`. An EOA carrying an EIP-7702 delegation designator (`0xef0100 ++ delegate`, 23 bytes) passed it, and a clone of it works: `initialize` runs the delegate's code. But the designator is a pointer, not code: the account holder can repoint or revoke it with a new authorization at any time, after clones exist, putting every clone into exactly the "delegates every call to nothing" state the guard exists to prevent, and voiding the open-salt claim that an occupied address holds the clone that was asked for initialized by the code that was asked for. The guard now also reads the first byte of the code and reverts with a new typed error `DelegatedImplementation` when it is `0xef`. EIP-3541 forbids deploying `0xef`-leading code, so an account holds it only as a 7702 designator; what passes is therefore exactly deployed contract code, which is what an EIP-1167 proxy assumes. One `EXTCODECOPY` of a single byte into scratch space, so the cost does not scale with implementation size. `testCheckImplementationCodeEip7702Designator` now pins the revert with the exact selector, and each clone entry point gets a test that a delegated implementation reverts with it and deploys nothing at the predicted address. Closes #73 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The guard now copies one byte of implementation code, so every clone path costs a few more gas; the two new entry-point tests are added. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
|
What changed
LibICloneableFactoryV4.checkImplementationCodenow rejects an implementation whose code begins with0xef, with a new typed errorDelegatedImplementation(), alongside the existingZeroImplementationCodeSize()for empty code.Why this is correct
An EIP-1167 proxy assumes its implementation holds deployed contract code: code that is fixed for the life of the account. The guard's stated purpose is to refuse an implementation whose clone would "delegate every call —
initializeincluded — to nothing", andpredictDeterministicAddressOpenSaltpromises that whatever occupies an open-salt address is "the clone that was asked for, initialized with the bytes that were asked for".An EOA carrying an EIP-7702 delegation designator (
0xef0100 ++ delegate, 23 bytes) hasEXTCODESIZE == 23, so a size-only guard admits it, and a clone of it works. But the designator is a pointer, not code: the account holder can repoint it or revoke it to empty with a new authorization at any time, after clones exist, by a party who is neither the deployer nor the clone's users. That reaches exactly the state the guard exists to prevent, on every clone ever made, and it voids the open-salt claim: the tuple(factory, implementation, salt, data)no longer fixes what code initialized the clone.EIP-3541 forbids deploying
0xef-leading code, so the only way an account holds it is the designator. Checking the first byte is therefore the precise test for "this is deployed contract code" and is oneEXTCODECOPYof a single byte into scratch space; cost does not scale with implementation size.The alternative reading in the issue (leave the guard size-only and document that a delegated EOA is accepted with mutable behaviour) would document the defect: the interface calls
implementation"the contract to clone" / "the reference bytecode cloned as a proxy", and a revocable pointer is neither.Migration
The concrete
CloneFactoryinrain.factory.deployinlines this library, so its bytecode changes and the next deploy there is a new pin. Already-deployed factories keep admitting delegated accounts; nothing on chain changes. Any caller that deliberately cloned a delegated account now getsDelegatedImplementation().Out of scope
foundry.tomlpinsevm_version = "cancun", which predates EIP-7702, so the tests store the designator withvm.etchand assert the guard; delegation execution semantics are not exercised.Closes #73
QA
testCheckImplementationCodeEip7702Designator(main's version asserted the designator PASSES; now expects the exactDelegatedImplementation()selector),testCloneDeterministicDelegatedImplementation,testCloneDeterministicOpenSaltDelegatedImplementation- each fails on base (M1 below is the base guard's behaviour for every designator, and all three fail under it; on main the two entry-point tests deploy a clone instead of reverting)src/lib/LibICloneableFactoryV4.sol:156firstByte == 0xef->== 0xee(reintroduces the defect for every designator) -> killed bytestCheckImplementationCodeEip7702Designator,testCloneDeterministicDelegatedImplementation,testCloneDeterministicOpenSaltDelegatedImplementation,testCheckImplementationCodeEtched(41 pass, 4 fail);:154byte(0, mload(0))->byte(1, ...)-> killed by the same three designator tests (42/3);:153extcodecopy(implementation, 0, 0, 1)->(implementation, 0, 1, 1)-> killed by the three designator tests plustestCheckImplementationCodeEtched(41/4);:156==->!=-> 21 fail incl.testCheckImplementationCodeContract,testCheckImplementationCodeEtchedand every clone flow test (24/21)0xef0100 || address(23 bytes) and EIP-3541 (no deployed code starts with0xef); theZeroImplementationCodeSizerationale and thepredictDeterministicAddressOpenSaltoccupancy claim in this repo's NatSpec; expected revert is the exact error selector, and the post-revert assertion ispredicted.code.length == 0🤖 Generated with Claude Code