I have a few suggestions or questions regarding the current spec.
1. Regarding the functionality of isValidSigner which is currently defined as:
/**
* @dev Returns a magic value indicating whether a given signer is authorized to act on behalf of the account
*
* MUST return the bytes4 magic value 0x523e3260 if the given signer is valid
*
* By default, the holder of the non-fungible token the account is bound to MUST be considered a valid
* signer
*
* Accounts MAY implement additional authorization logic which invalidates the holder as a
* signer or grants signing permissions to other non-holder accounts
*
* @param signer The address to check signing authorization for
* @param context Additional data used to determine whether the signer is valid
* @return magicValue Magic value indicating whether the signer is valid
*/
function isValidSigner(address signer, bytes calldata context)
I believe it should be amended to specify this method SHOULD NOT revert in case the signer is not valid.
I saw that the reference implementation does not revert in case the signature is invalid, but it is important to specify in this case (as well as other cases) the unhappy path the execution takes.
Consider adding (or I can make a PR) with the following specification:
* MUST return the bytes4 magic value 0x523e3260 if the given signer is valid
* SHOULD NOT revert in case the given signer is invalid
* SHOULD return 0x00000000 in case the given signer is invalid
I want to prevent different types of implementations which would require a smart contract interacting with ERC6551Account implementations where it expects that some calls fail (revert) and some return bytes4(0).
2. Add event to signal correct execution.
The interface IERC6551Executable does not emit any event in case of correct execution.
Consider adding an event which logs the correct execution as well as the returned values.
A possible option could look like:
event Execute(
address indexed to,
uint256 value,
bytes data,
uint256 operation,
bytes response
);
Additionally, the reference implementation should be updated to emit this event.
3. Gas optimization in case of reverted execution.
Current reference implementation of ERC6551Account execute is:
function execute(
address to,
uint256 value,
bytes calldata data,
uint256 operation
) external payable returns (bytes memory result) {
require(_isValidSigner(msg.sender), "Invalid signer");
require(operation == 0, "Only call operations are supported");
++state;
bool success;
(success, result) = to.call{value: value}(data);
if (!success) {
assembly {
revert(add(result, 32), mload(result))
}
}
}
If the execution is reverted, the whole contract state is also reverted, thus the state storage variable remains unchanged. By moving the ++state at the end of the method, you reduce the gas used in case the execution is reverted.
However, it is important to note that if the external call eventually calls back into ERC6551Account, the state will not be changed (yet).
This gas optimization might not be a good idea, considering the state does not change until the full execution finalizes. This is something up for discussion. When should the state be updated?
4. Updating the state in case ether is received
This is something that is also up for discussion. Should the contract state be updated in case ether is received?
Thus, it is worth considering if the execution of receive() should update the state and in what conditions.
Attach the current token bound account state to the marketplace order. If the state of the account has changed since the order was placed, consider the offer void. This functionality would need to be supported at the marketplace level.
Considering the Security Considerations, specifically, if a trade would be made invalid by changing the contract state, a simple call to receive() (with 0 ether or even 1 wei) would update and invalidate the Account state.
I am happy to think about these suggestions further, discuss them or even open PRs if any of them go through.
