π¨ People continue to invent new ways to create security issues.
I found out about ClonesWithImmutableArgs, and I found a structural problem in this pattern.
It doesn't mean all contracts will be exploitable, but we must know when they are vulnerable.
twitter.com/boredGenius/status/1484618024564891648
Understanding how the code you import works is incredibly important.
This new pattern creates clones of a contract, simulating immutable args using `calldata` and lots of assembly.
The problem is the "immutable" args are just "calldata" args (not really immutable). This means that ANYONE can call the contract with a specially crafted calldata.
Doing that, your implementation will parse that calldata and use the arguments.
Thus the "immutable" args are not actually immutable.
I assume the creator of this pattern chose the word "immutable" because the arguments exist in the code. But there is a difference that makes this whole thing exploitable.
First, we need to understand how the pattern works.
The whole implementation uses another common pattern known as the Beacon Proxy Pattern.
In the Beacon Proxy Pattern, you have a pair of contracts one is the actual implementation, and the other one is the beacon pointing to the implementation.
The Beacon points to the implementation, and it holds the contract storage.
This pattern is a simple way of deploying multiple "thin" contracts, each with its own storage that behaves identically to all others (has the same implementation).
In the Beacon Proxy pattern, the users interact with the Beacons and the Beacon delegates the execution to the Implementation.
This pattern is very similar to Clones With Immutable Args. The difference is that some arguments are passed down to the Implementation through calldata
If you want to understand how this is done, it's not very easy, but I'll help you.
It's not easy because it's assembly. But follow below π
The immutable args are sent to the contract when a new contract is deployed and saved in the bytecode (similar to actual immutable variables).
Thus, the Beacon is deployed when the `.clone(implementation, args)` method is called.
The Beacon holds the "immutable args" and all execution is forwarded (with delegatecall) to the `implementation`, adding the "immutable args" at the end (`extra`).
At the other end, the implementation picks up the arguments from the calldata using specially crafted functions.
The function below reads an address.
The problem appears when the implementation believes the address should be trusted and specifically when it delegates more execution to it.
Your implementation can do whatever it needs with the arguments. And sometimes you might need to delegatecall to the received address.
Follow this example using the ClonesWithImmutableArgs pattern.
Have a look and tell me if there is something wrong with it.
Did you see the problem? Is it obvious?
I hope it's a bit more obvious now that you understand how this pattern works.
You can't trust the address returned by `_getArgAddress(0)`. Why?
In theory, this should be a trusted address that the Beacon forwards to you.
But anyone can call this contract and add what address they want in the calldata.
This means that CodeRunner will delegatecall to an address controlled by the attacker.
You might say this isn't a problem.
However, the delegated code can choose to selfdestruct. And because all execution happens in the implementation, the implementation selfdestructs.
This means that all beacons point to a self-destructed contract.
You don't change the Beacon's storage but make all Beacons unusable.
I believe it's extremely important to understand how patterns work behind the scenes and make sure your code doesn't open up doors that should stay closed.
Similar bugs in the wild below π
Upgradability again, proxy pattern that delegates calls.
blog.trailofbits.com/2020/12/16/breaking-aave-upgradeability/
Next is @AstariaXYZ using the same ClonesWithImmutableArgs pattern.
twitter.com/apoorvlathey/status/1671308180545241088
I am not saying we should stop using code from other devs, but it's really difficult to include code and know exactly how it works.
Most of the time we must trust something, but we shouldn't trust Solidity code.