Skip to content
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

Jump to bottom

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

Merged
merged 21 commits into from
Mar 6, 2024
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
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
5 changes: 5 additions & 0 deletions .changeset/chilled-walls-develop.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`Clones`: Add version of `clone` and `cloneDeterministic` that support sending value at creation.
5 changes: 5 additions & 0 deletions .changeset/strong-singers-talk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`Errors`: New library of common custom errors.
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ $ npm install @openzeppelin/contracts
$ forge install OpenZeppelin/openzeppelin-contracts
```

Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.`
Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.`

### Usage

Expand Down
3 changes: 2 additions & 1 deletion contracts/metatx/ERC2771Forwarder.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {ECDSA} from "../utils/cryptography/ECDSA.sol";
import {EIP712} from "../utils/cryptography/EIP712.sol";
import {Nonces} from "../utils/Nonces.sol";
import {Address} from "../utils/Address.sol";
import {Errors} from "../utils/Errors.sol";

/**
* @dev A forwarder compatible with ERC-2771 contracts. See {ERC2771Context}.
Expand Down Expand Up @@ -132,7 +133,7 @@ contract ERC2771Forwarder is EIP712, Nonces {
}

if (!_execute(request, true)) {
revert Address.FailedInnerCall();
revert Errors.FailedInnerCall();
}
}

Expand Down
41 changes: 34 additions & 7 deletions contracts/proxy/Clones.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

pragma solidity ^0.8.20;

import {Errors} from "../utils/Errors.sol";

/**
* @dev https://eips.ethereum.org/EIPS/eip-1167[ERC-1167] is a standard for
* deploying minimal proxy contracts, also known as "clones".
Expand All @@ -16,27 +18,34 @@ pragma solidity ^0.8.20;
*/
library Clones {
/**
* @dev A clone instance deployment failed.
* @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.
*/
error ERC1167FailedCreateClone();
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.
*/
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
function clone(address implementation) internal returns (address instance) {
function clone(address implementation, uint256 value) internal returns (address instance) {
Copy link
Member

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

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

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

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

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

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.

if (address(this).balance < value) {
revert Errors.InsufficientBalance(address(this).balance, 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();
revert Errors.FailedDeployment();
}
}

Expand All @@ -48,17 +57,35 @@ 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.
*/
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
function cloneDeterministic(
address implementation,
bytes32 salt,
uint256 value
) internal returns (address instance) {
if (address(this).balance < value) {
revert Errors.InsufficientBalance(address(this).balance, 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 := create2(0, 0x09, 0x37, salt)
instance := create2(value, 0x09, 0x37, salt)
}
if (instance == address(0)) {
revert ERC1167FailedCreateClone();
revert Errors.FailedDeployment();
}
}

Expand Down
30 changes: 11 additions & 19 deletions contracts/utils/Address.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,17 @@

pragma solidity ^0.8.20;

import {Errors} from "./Errors.sol";

/**
* @dev Collection of functions related to the address type
*/
library Address {
/**
* @dev The ETH balance of the account is not enough to perform the operation.
*/
error AddressInsufficientBalance(address account);

/**
* @dev There's no code at `target` (it is not a contract).
*/
error AddressEmptyCode(address target);

/**
* @dev A call to an address target failed. The target may have reverted.
*/
error FailedInnerCall();

/**
* @dev Replacement for Solidity's `transfer`: sends `amount` wei to
* `recipient`, forwarding all available gas and reverting on errors.
Expand All @@ -40,12 +32,12 @@ library Address {
*/
function sendValue(address payable recipient, uint256 amount) internal {
if (address(this).balance < amount) {
revert AddressInsufficientBalance(address(this));
revert Errors.InsufficientBalance(address(this).balance, amount);
}

(bool success, ) = recipient.call{value: amount}("");
if (!success) {
revert FailedInnerCall();
revert Errors.FailedInnerCall();
}
}

Expand All @@ -57,7 +49,7 @@ library Address {
* If `target` reverts with a revert reason or custom error, it is bubbled
* up by this function (like regular Solidity function calls). However, if
* the call reverted with no returned reason, this function reverts with a
* {FailedInnerCall} error.
* {Errors.FailedInnerCall} error.
*
* Returns the raw returned data. To convert to the expected return value,
* use https://solidity.readthedocs.io/en/latest/units-and-global-variables.html?highlight=abi.decode#abi-encoding-and-decoding-functions[`abi.decode`].
Expand All @@ -82,7 +74,7 @@ library Address {
*/
function functionCallWithValue(address target, bytes memory data, uint256 value) internal returns (bytes memory) {
if (address(this).balance < value) {
revert AddressInsufficientBalance(address(this));
revert Errors.InsufficientBalance(address(this).balance, value);
}
(bool success, bytes memory returndata) = target.call{value: value}(data);
return verifyCallResultFromTarget(target, success, returndata);
Expand All @@ -108,8 +100,8 @@ library Address {

/**
* @dev Tool to verify that a low level call to smart-contract was successful, and reverts if the target
* was not a contract or bubbling up the revert reason (falling back to {FailedInnerCall}) in case of an
* unsuccessful call.
* was not a contract or bubbling up the revert reason (falling back to {Errors.FailedInnerCall}) in case
* of an unsuccessful call.
*/
function verifyCallResultFromTarget(
address target,
Expand All @@ -130,7 +122,7 @@ library Address {

/**
* @dev Tool to verify that a low level call was successful, and reverts if it wasn't, either by bubbling the
* revert reason or with a default {FailedInnerCall} error.
* revert reason or with a default {Errors.FailedInnerCall} error.
*/
function verifyCallResult(bool success, bytes memory returndata) internal pure returns (bytes memory) {
if (!success) {
Expand All @@ -141,7 +133,7 @@ library Address {
}

/**
* @dev Reverts with returndata if present. Otherwise reverts with {FailedInnerCall}.
* @dev Reverts with returndata if present. Otherwise reverts with {Errors.FailedInnerCall}.
*/
function _revert(bytes memory returndata) private pure {
// Look for revert reason and bubble it up if present
Expand All @@ -153,7 +145,7 @@ library Address {
revert(add(32, returndata), returndata_size)
}
} else {
revert FailedInnerCall();
revert Errors.FailedInnerCall();
}
}
}
16 changes: 4 additions & 12 deletions contracts/utils/Create2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

pragma solidity ^0.8.20;

import {Errors} from "./Errors.sol";

/**
* @dev Helper to make usage of the `CREATE2` EVM opcode easier and safer.
* `CREATE2` can be used to compute in advance the address where a smart
Expand All @@ -13,21 +15,11 @@ pragma solidity ^0.8.20;
* information.
*/
library Create2 {
/**
* @dev Not enough balance for performing a CREATE2 deploy.
*/
error Create2InsufficientBalance(uint256 balance, uint256 needed);

/**
* @dev There's no code to deploy.
*/
error Create2EmptyBytecode();

/**
* @dev The deployment failed.
*/
error Create2FailedDeployment();

/**
* @dev Deploys a contract using `CREATE2`. The address where the contract
* will be deployed can be known in advance via {computeAddress}.
Expand All @@ -44,7 +36,7 @@ library Create2 {
*/
function deploy(uint256 amount, bytes32 salt, bytes memory bytecode) internal returns (address addr) {
if (address(this).balance < amount) {
revert Create2InsufficientBalance(address(this).balance, amount);
revert Errors.InsufficientBalance(address(this).balance, amount);
}
if (bytecode.length == 0) {
revert Create2EmptyBytecode();
Expand All @@ -54,7 +46,7 @@ library Create2 {
addr := create2(amount, add(bytecode, 0x20), mload(bytecode), salt)
}
if (addr == address(0)) {
revert Create2FailedDeployment();
revert Errors.FailedDeployment();
}
}

Expand Down
26 changes: 26 additions & 0 deletions contracts/utils/Errors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

/**
* @dev Collection of common custom errors used in multiple contracts
*
* IMPORTANT: Backwards compatibility is not guaranteed in future versions of the library.
* It is recommended to avoid relying on the error API for critical functionality.
*/
library Errors {
/**
* @dev The ETH balance of the account is not enough to perform the operation.
*/
error InsufficientBalance(uint256 balance, uint256 needed);

/**
* @dev A call to an address target failed. The target may have reverted.
*/
error FailedInnerCall();
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be more generic to just call it FailedCall. However, I'm realizing that the main information this error provides is that it was a subcall that failed.

I'd like to give a second thought to FailedInnerCall. Not sure if Inner is the best. Would it make more sense to call it FailedSubCall?

Copy link
Collaborator

Choose a reason for hiding this comment

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

FailedCall, FailedInnerCall and FailedSubCall are all good to me. My favorite is FailedCall, because simplicity ... but all are good honestly.

Copy link
Collaborator

@Amxx Amxx 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.

I also like the symetry of FailedCall <> FailedDeployment

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this error is also emitted when the call that fails is actually a static call or a delegate call

Copy link
Member

Choose a reason for hiding this comment

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

I also like the symetry of FailedCall <> FailedDeployment

Let's do FailedCall, I think it covers the static and delegate call as well.


/**
* @dev The deployment failed.
*/
error FailedDeployment();
}
16 changes: 8 additions & 8 deletions scripts/upgradeable/upgradeable.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ index ff596b0c3..000000000
-<!-- Make sure that you have reviewed the OpenZeppelin Contracts Contributor Guidelines. -->
-<!-- https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/CONTRIBUTING.md -->
diff --git a/README.md b/README.md
index 35083bc6e..05cf4fc27 100644
index fa7b4e31e..4799b6376 100644
--- a/README.md
+++ b/README.md
@@ -19,6 +19,9 @@
Expand Down Expand Up @@ -89,8 +89,8 @@ index 35083bc6e..05cf4fc27 100644
+$ forge install OpenZeppelin/openzeppelin-contracts-upgradeable
```

-Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.`
+Add `@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/` in `remappings.txt.`
-Add `@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/` in `remappings.txt.`
+Add `@openzeppelin/contracts-upgradeable/=lib/openzeppelin-contracts-upgradeable/contracts/` in `remappings.txt.`

### Usage

Expand All @@ -110,15 +110,15 @@ index 35083bc6e..05cf4fc27 100644
}
```
diff --git a/contracts/package.json b/contracts/package.json
index 6ab89138a..ece834a44 100644
index 845e8c403..8dc181b91 100644
--- a/contracts/package.json
+++ b/contracts/package.json
@@ -1,5 +1,5 @@
{
- "name": "@openzeppelin/contracts",
+ "name": "@openzeppelin/contracts-upgradeable",
"description": "Secure Smart Contract library for Solidity",
"version": "5.0.1",
"version": "5.0.2",
"files": [
@@ -13,7 +13,7 @@
},
Expand Down Expand Up @@ -307,7 +307,7 @@ index 77c4c8990..602467f40 100644
}
}
diff --git a/package.json b/package.json
index ec2c44ced..46eedc98f 100644
index c4b358e10..96ab2559c 100644
--- a/package.json
+++ b/package.json
@@ -32,7 +32,7 @@
Expand All @@ -328,7 +328,7 @@ index 304d1386a..a1cd63bee 100644
+@openzeppelin/contracts-upgradeable/=contracts/
+@openzeppelin/contracts/=lib/openzeppelin-contracts/contracts/
diff --git a/test/utils/cryptography/EIP712.test.js b/test/utils/cryptography/EIP712.test.js
index 166038b36..268e0d29d 100644
index 2b6e7fa97..268e0d29d 100644
--- a/test/utils/cryptography/EIP712.test.js
+++ b/test/utils/cryptography/EIP712.test.js
@@ -47,27 +47,6 @@ describe('EIP712', function () {
Expand All @@ -346,7 +346,7 @@ index 166038b36..268e0d29d 100644
- const clone = await factory
- .$clone(this.eip712)
- .then(tx => tx.wait())
- .then(receipt => receipt.logs.find(ev => ev.fragment.name == 'return$clone').args.instance)
- .then(receipt => receipt.logs.find(ev => ev.fragment.name == 'return$clone_address').args.instance)
- .then(address => ethers.getContractAt('$EIP712Verifier', address));
-
- const expectedDomain = { ...this.domain, verifyingContract: clone.target };
Expand Down
Loading
Loading