Skip to content

Add ability to create clones with initial value in Clones.sol #4936

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ernestognw merged 21 commits into from
Mar 6, 2024
Merged

Add ability to create clones with initial value in Clones.sol #4936

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
c1d093d
Add Clones lib ability to support non-zero value
k06a Feb 29, 2024
1052cb1
add Clones testing with value & create utils/Errors.sol
Amxx Mar 1, 2024
6aab488
fix lint
Amxx Mar 1, 2024
9618bcd
simplify
Amxx Mar 1, 2024
d0f9186
fix tests
Amxx Mar 1, 2024
a41bc51
fix upgradeable patch
Amxx Mar 1, 2024
4676598
Apply PR suggestions
ernestognw Mar 5, 2024
463ff75
Apply PR suggestions
ernestognw Mar 5, 2024
132bccb
Fix docs
ernestognw Mar 5, 2024
fcc327b
add warning
Amxx Mar 5, 2024
d2fcabf
Rename FailedInnerCall → FailedCall
Amxx Mar 5, 2024
e8eb625
Apply suggestions
ernestognw Mar 5, 2024
3f820b2
Merge branch 'feature/clones-value' of https://github.com/1inch/openz…
Amxx Mar 5, 2024
27015a6
fix lint
Amxx Mar 5, 2024
79c4c86
Mer branch 'feature/clones-value' of https://github.com/1inch/openzep…
Amxx Mar 5, 2024
6942c46
re-fix lint
Amxx Mar 5, 2024
524ccb6
fix merge conflict
Amxx Mar 5, 2024
f02beb7
Document error changes
ernestognw Mar 5, 2024
64d394f
Merge branch 'master' into feature/clones-value
ernestognw Mar 6, 2024
a24f2b2
Merge branch 'master' into feature/clones-value
ernestognw Mar 6, 2024
528ebdc
fixing: how could that happen? good thing we have tests!
Amxx Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions contracts/proxy/Clones.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,23 @@ library Clones {
* This function uses the create opcode, which should never revert.
*/
function clone(address implementation) internal returns (address instance) {
return clone(implementation, 0);
}

/**
* @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`.
*
* This function uses the create opcode, which should never revert.
*/
function clone(address implementation, uint256 value) internal returns (address instance) {
Copy link
Member

@ernestognw ernestognw Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that this accepts value, a factory may get locked if it doesn't accept ETH. Shall we add a note? @Amxx

Copy link
Member

@ernestognw ernestognw Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this enables a couple of attack vectors:

  • Can the factory be griefed (e.g. steal eth from it)?
  • Can the factory stop receiving ETH to fund its operation?
  • Can the factory be partially DoS'd if all payable entrypoints are restricted to a single entity?

I start to feel we didn't do this in the past because it's easy to fuck it up. We can still have the function, I just want to make sure we document the risks correctly.

Copy link
Collaborator

@Amxx Amxx Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, that is factory design, which is beyond the scope of this library. Most factory will not use value. Those that do need to take care of that.

IMO it's like Address library (that can do call with value) and TrustedForwarder (that can trigger that call). We don't have any warning in Address about the value issue.

Copy link
Member

@ernestognw ernestognw Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, indeed, we don't have a warning in Address nor ERC2771Forwarder, but the main difference is the use case:

  • ERC2771Forwarder's balance should be always zero since every call forwarded goes along with the value and everything else is refunded
  • Address is more generic and it's fine if the caller doesn't have enough balance

Although Clones is very similar to Address, it's more likely users will need to consider designing their factory in such a way that always has enough balance. Even though that's factory design, I think it makes sense to make a simple recommendation:

NOTE: Using a non-zero value at creation will require the contract using this function (e.g. a factory)
to always have enough balance for new deployments. Consider exposing this function under a payable
method.

Copy link
Collaborator

@Amxx Amxx Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That warning is fine

Copy link
Collaborator

@Amxx Amxx Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, 99% or devs won't ever use this library, and out of the 1% remining, 99% will not use value.

That leaves us with 0.01%, which is basically only @k06a. Unless he decides to not test/audit its code, I think nobody will ever need that warning :)

Copy link
Member

@ernestognw ernestognw Mar 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah probably true 😂, but ideally we would get more experienced developers like @k06a in the long run.

Think that with the Account Abstraction trend, I'd expect more factories to deploy accounts, and therefore more developers requiring initial value.

/// @solidity memory-safe-assembly
assembly {
// Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes
// of the `implementation` address with the bytecode before the address.
mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000))
// Packs the remaining 17 bytes of `implementation` with the bytecode after the address.
mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3))
instance := create(0, 0x09, 0x37)
instance := create(value, 0x09, 0x37)
}
if (instance == address(0)) {
revert ERC1167FailedCreateClone();
Expand All @@ -48,14 +57,25 @@ library Clones {
* the clones cannot be deployed twice at the same address.
*/
function cloneDeterministic(address implementation, bytes32 salt) internal returns (address instance) {
return cloneDeterministic(implementation, salt, 0);
}

/**
* @dev Deploys and returns the address of a clone that mimics the behaviour of `implementation`.
*
* This function uses the create2 opcode and a `salt` to deterministically deploy
* the clone. Using the same `implementation` and `salt` multiple time will revert, since
* the clones cannot be deployed twice at the same address.
*/
function cloneDeterministic(address implementation, bytes32 salt, uint256 value) internal returns (address instance) {
/// @solidity memory-safe-assembly
assembly {
// Cleans the upper 96 bits of the `implementation` word, then packs the first 3 bytes
// of the `implementation` address with the bytecode before the address.
mstore(0x00, or(shr(0xe8, shl(0x60, implementation)), 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000))
// Packs the remaining 17 bytes of `implementation` with the bytecode after the address.
mstore(0x20, or(shl(0x78, implementation), 0x5af43d82803e903d91602b57fd5bf3))
instance := create2(0, 0x09, 0x37, salt)
instance := create2(value, 0x09, 0x37, salt)
}
if (instance == address(0)) {
revert ERC1167FailedCreateClone();
Expand Down