Smart Account contracts: audit fixes (#396)

* rename account factory callbacks

* Create and use AccountPermissions

* Fix [M-3] Native tokens can get locked in ManagedAccountFactory

* cleanup

* Fix [L-1, L-2]

* Fix [Q-1] Potentially susceptible to signature malleability

* Fix [Q-2, Q-3, Q-4, G-1]

* update tests

* Fix isValidSignature logic for non-admins

* Fix Appendix A-2

* Fix Appendix A-3

* make modifiers virtual and return result from internal _call function

* Add ContractMetadata to all account factories

* Add some checks to factory callbacks

* gas benchmarks

* docs update

* pkg release

* pkg update

* pkg update

---------

Co-authored-by: Joaquim Verges <joaquim.verges@gmail.com>

contracts/dynamic-contracts/extension/AccountPermissions.sol

@@ -0,0 +1,203 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.0;
+
+/// @author thirdweb
+
+import "../../extension/interface/IAccountPermissions.sol";
+import "../../openzeppelin-presets/utils/cryptography/EIP712.sol";
+import "../../openzeppelin-presets/utils/structs/EnumerableSet.sol";
+
+library AccountPermissionsStorage {
+    bytes32 public constant ACCOUNT_PERMISSIONS_STORAGE_POSITION = keccak256("account.permissions.storage");
+
+    struct Data {
+        /// @dev Map from address => whether the address is an admin.
+        mapping(address => bool) isAdmin;
+        /// @dev Map from keccak256 hash of a role => active restrictions for that role.
+        mapping(bytes32 => IAccountPermissions.RoleStatic) roleRestrictions;
+        /// @dev Map from address => the role held by that address.
+        mapping(address => bytes32) roleOfAccount;
+        /// @dev Mapping from a signed request UID => whether the request is processed.
+        mapping(bytes32 => bool) executed;
+        /// @dev Map from keccak256 hash of a role to its approved targets.
+        mapping(bytes32 => EnumerableSet.AddressSet) approvedTargets;
+        /// @dev map from keccak256 hash of a role to its members' data. See {RoleMembers}.
+        mapping(bytes32 => EnumerableSet.AddressSet) roleMembers;
+    }
+
+    function accountPermissionsStorage() internal pure returns (Data storage accountPermissionsData) {
+        bytes32 position = ACCOUNT_PERMISSIONS_STORAGE_POSITION;
+        assembly {
+            accountPermissionsData.slot := position
+        }
+    }
+}
+
+abstract contract AccountPermissions is IAccountPermissions, EIP712 {
+    using ECDSA for bytes32;
+    using EnumerableSet for EnumerableSet.AddressSet;
+
+    bytes32 private constant TYPEHASH =
+        keccak256(
+            "RoleRequest(bytes32 role,address target,uint8 action,uint128 validityStartTimestamp,uint128 validityEndTimestamp,bytes32 uid)"
+        );
+
+    modifier onlyAdmin() virtual {
+        require(isAdmin(msg.sender), "AccountPermissions: caller is not an admin");
+        _;
+    }
+
+    /*///////////////////////////////////////////////////////////////
+                            External functions
+    //////////////////////////////////////////////////////////////*/
+
+    /// @notice Adds / removes an account as an admin.
+    function setAdmin(address _account, bool _isAdmin) external virtual onlyAdmin {
+        _setAdmin(_account, _isAdmin);
+    }
+
+    /// @notice Sets the restrictions for a given role.
+    function setRoleRestrictions(RoleRestrictions calldata _restrictions) external virtual onlyAdmin {
+        require(_restrictions.role != bytes32(0), "AccountPermissions: role cannot be empty");
+
+        AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
+        data.roleRestrictions[_restrictions.role] = RoleStatic(
+            _restrictions.role,
+            _restrictions.maxValuePerTransaction,
+            _restrictions.startTimestamp,
+            _restrictions.endTimestamp
+        );
+
+        uint256 len = _restrictions.approvedTargets.length;
+        delete data.approvedTargets[_restrictions.role];
+        for (uint256 i = 0; i < len; i++) {
+            data.approvedTargets[_restrictions.role].add(_restrictions.approvedTargets[i]);
+        }
+
+        emit RoleUpdated(_restrictions.role, _restrictions);
+    }
+
+    /// @notice Grant / revoke a role from a given signer.
+    function changeRole(RoleRequest calldata _req, bytes calldata _signature) external virtual {
+        require(_req.role != bytes32(0), "AccountPermissions: role cannot be empty");
+        require(
+            _req.validityStartTimestamp < block.timestamp && block.timestamp < _req.validityEndTimestamp,
+            "AccountPermissions: invalid validity period"
+        );
+
+        (bool success, address signer) = verifyRoleRequest(_req, _signature);
+
+        require(success, "AccountPermissions: invalid signature");
+
+        AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
+        data.executed[_req.uid] = true;
+
+        if (_req.action == RoleAction.GRANT) {
+            data.roleOfAccount[_req.target] = _req.role;
+            data.roleMembers[_req.role].add(_req.target);
+        } else {
+            delete data.roleOfAccount[_req.target];
+            data.roleMembers[_req.role].remove(_req.target);
+        }
+
+        emit RoleAssignment(_req.role, _req.target, signer, _req);
+    }
+
+    /*///////////////////////////////////////////////////////////////
+                            View functions
+    //////////////////////////////////////////////////////////////*/
+
+    /// @notice Returns whether the given account is an admin.
+    function isAdmin(address _account) public view virtual returns (bool) {
+        AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
+        return data.isAdmin[_account];
+    }
+
+    /// @notice Returns the role held by a given account along with its restrictions.
+    function getRoleRestrictionsForAccount(address _account) external view virtual returns (RoleRestrictions memory) {
+        AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
+        bytes32 role = data.roleOfAccount[_account];
+        RoleStatic memory roleRestrictions = data.roleRestrictions[role];
+
+        return
+            RoleRestrictions(
+                role,
+                data.approvedTargets[role].values(),
+                roleRestrictions.maxValuePerTransaction,
+                roleRestrictions.startTimestamp,
+                roleRestrictions.endTimestamp
+            );
+    }
+
+    /// @notice Returns the role restrictions for a given role.
+    function getRoleRestrictions(bytes32 _role) external view virtual returns (RoleRestrictions memory) {
+        AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
+        RoleStatic memory roleRestrictions = data.roleRestrictions[_role];
+
+        return
+            RoleRestrictions(
+                _role,
+                data.approvedTargets[_role].values(),
+                roleRestrictions.maxValuePerTransaction,
+                roleRestrictions.startTimestamp,
+                roleRestrictions.endTimestamp
+            );
+    }
+
+    /// @notice Returns all accounts that have a role.
+    function getAllRoleMembers(bytes32 _role) external view virtual returns (address[] memory) {
+        AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
+        return data.roleMembers[_role].values();
+    }
+
+    /// @dev Verifies that a request is signed by an authorized account.
+    function verifyRoleRequest(RoleRequest calldata req, bytes calldata signature)
+        public
+        view
+        virtual
+        returns (bool success, address signer)
+    {
+        AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
+        signer = _recoverAddress(req, signature);
+        success = !data.executed[req.uid] && isAdmin(signer);
+    }
+
+    /*///////////////////////////////////////////////////////////////
+                        Internal functions
+    //////////////////////////////////////////////////////////////*/
+
+    /// @notice Runs after every `changeRole` run.
+    function _afterChangeRole(RoleRequest calldata _req) internal virtual;
+
+    /// @notice Makes the given account an admin.
+    function _setAdmin(address _account, bool _isAdmin) internal virtual {
+        AccountPermissionsStorage.Data storage data = AccountPermissionsStorage.accountPermissionsStorage();
+        data.isAdmin[_account] = _isAdmin;
+
+        emit AdminUpdated(_account, _isAdmin);
+    }
+
+    /// @dev Returns the address of the signer of the request.
+    function _recoverAddress(RoleRequest calldata _req, bytes calldata _signature)
+        internal
+        view
+        virtual
+        returns (address)
+    {
+        return _hashTypedDataV4(keccak256(_encodeRequest(_req))).recover(_signature);
+    }
+
+    /// @dev Encodes a request for recovery of the signer in `recoverAddress`.
+    function _encodeRequest(RoleRequest calldata _req) internal pure virtual returns (bytes memory) {
+        return
+            abi.encode(
+                TYPEHASH,
+                _req.role,
+                _req.target,
+                _req.action,
+                _req.validityStartTimestamp,
+                _req.validityEndTimestamp,
+                _req.uid
+            );
+    }
+}

contracts/extension/interface/IAccountPermissions.sol

@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: Apache-2.0
+pragma solidity ^0.8.0;
+
+/// @author thirdweb
+
+interface IAccountPermissions {
+    /*///////////////////////////////////////////////////////////////
+                                Types
+    //////////////////////////////////////////////////////////////*/
+
+    /// @notice Roles can be granted or revoked by an authorized party.
+    enum RoleAction {
+        GRANT,
+        REVOKE
+    }
+
+    /**
+     *  @notice The payload that must be signed by an authorized wallet to grant / revoke a role.
+     *
+     *  @param role The role to grant / revoke.
+     *  @param target The address to grant / revoke the role from.
+     *  @param action Whether to grant or revoke the role.
+     *  @param validityStartTimestamp The UNIX timestamp at and after which a signature is valid.
+     *  @param validityEndTimestamp The UNIX timestamp at and after which a signature is invalid/expired.
+     *  @param uid A unique non-repeatable ID for the payload.
+     */
+    struct RoleRequest {
+        bytes32 role;
+        address target;
+        RoleAction action;
+        uint128 validityStartTimestamp;
+        uint128 validityEndTimestamp;
+        bytes32 uid;
+    }
+
+    /**
+     *  @notice Restrictions that can be applied to a given role.
+     *
+     *  @param role The unique role identifier.
+     *  @param approvedTargets The list of approved targets that a role holder can call using the smart wallet.
+     *  @param maxValuePerTransaction The maximum value that can be transferred by a role holder in a single transaction.
+     *  @param startTimestamp The UNIX timestamp at and after which a role holder can call the approved targets.
+     *  @param endTimestamp The UNIX timestamp at and after which a role holder can no longer call the approved targets.
+     */
+    struct RoleRestrictions {
+        bytes32 role;
+        address[] approvedTargets;
+        uint256 maxValuePerTransaction;
+        uint128 startTimestamp;
+        uint128 endTimestamp;
+    }
+
+    /**
+     *  @notice Internal struct for storing roles without approved targets
+     *
+     *  @param role The unique role identifier.
+     *  @param maxValuePerTransaction The maximum value that can be transferred by a role holder in a single transaction.
+     *  @param startTimestamp The UNIX timestamp at and after which a role holder can call the approved targets.
+     *  @param endTimestamp The UNIX timestamp at and after which a role holder can no longer call the approved targets.
+     */
+    struct RoleStatic {
+        bytes32 role;
+        uint256 maxValuePerTransaction;
+        uint128 startTimestamp;
+        uint128 endTimestamp;
+    }
+
+    /*///////////////////////////////////////////////////////////////
+                                Events
+    //////////////////////////////////////////////////////////////*/
+
+    /// @notice Emitted when the restrictions for a given role are updated.
+    event RoleUpdated(bytes32 indexed role, RoleRestrictions restrictions);
+
+    /// @notice Emitted when a role is granted / revoked by an authorized party.
+    event RoleAssignment(bytes32 indexed role, address indexed account, address indexed signer, RoleRequest request);
+
+    /// @notice Emitted when an admin is set or removed.
+    event AdminUpdated(address indexed account, bool isAdmin);
+
+    /*///////////////////////////////////////////////////////////////
+                            View functions
+    //////////////////////////////////////////////////////////////*/
+
+    /// @notice Returns whether the given account is an admin.
+    function isAdmin(address account) external view returns (bool);
+
+    /// @notice Returns the role held by a given account along with its restrictions.
+    function getRoleRestrictionsForAccount(address account) external view returns (RoleRestrictions memory role);
+
+    /// @notice Returns the role restrictions for a given role.
+    function getRoleRestrictions(bytes32 role) external view returns (RoleRestrictions memory restrictions);
+
+    /// @notice Returns all accounts that have a role.
+    function getAllRoleMembers(bytes32 role) external view returns (address[] memory members);
+
+    /// @dev Verifies that a request is signed by an authorized account.
+    function verifyRoleRequest(RoleRequest calldata req, bytes calldata signature)
+        external
+        view
+        returns (bool success, address signer);
+
+    /*///////////////////////////////////////////////////////////////
+                            External functions
+    //////////////////////////////////////////////////////////////*/
+
+    /// @notice Adds / removes an account as an admin.
+    function setAdmin(address account, bool isAdmin) external;
+
+    /// @notice Sets the restrictions for a given role.
+    function setRoleRestrictions(RoleRestrictions calldata role) external;
+
+    /// @notice Grant / revoke a role from a given signer.
+    function changeRole(RoleRequest calldata req, bytes calldata signature) external;
+}

contracts/openzeppelin-presets/proxy/Clones.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: MIT
-// OpenZeppelin Contracts (last updated v4.7.0) (proxy/Clones.sol)
+// OpenZeppelin Contracts (last updated v4.8.0) (proxy/Clones.sol)
 
 pragma solidity ^0.8.0;
 
@@ -25,11 +25,12 @@ library Clones {
     function clone(address implementation) internal returns (address instance) {
         /// @solidity memory-safe-assembly
         assembly {
-            let ptr := mload(0x40)
-            mstore(ptr, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)
-            mstore(add(ptr, 0x14), shl(0x60, implementation))
-            mstore(add(ptr, 0x28), 0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000)
-            instance := create(0, ptr, 0x37)
+            // 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)
         }
         require(instance != address(0), "ERC1167: create failed");
     }
@@ -44,11 +45,12 @@ library Clones {
     function cloneDeterministic(address implementation, bytes32 salt) internal returns (address instance) {
         /// @solidity memory-safe-assembly
         assembly {
-            let ptr := mload(0x40)
-            mstore(ptr, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)
-            mstore(add(ptr, 0x14), shl(0x60, implementation))
-            mstore(add(ptr, 0x28), 0x5af43d82803e903d91602b57fd5bf30000000000000000000000000000000000)
-            instance := create2(0, ptr, 0x37, salt)
+            // 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)
         }
         require(instance != address(0), "ERC1167: create2 failed");
     }
@@ -64,13 +66,13 @@ library Clones {
         /// @solidity memory-safe-assembly
         assembly {
             let ptr := mload(0x40)
-            mstore(ptr, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73000000000000000000000000)
-            mstore(add(ptr, 0x14), shl(0x60, implementation))
-            mstore(add(ptr, 0x28), 0x5af43d82803e903d91602b57fd5bf3ff00000000000000000000000000000000)
-            mstore(add(ptr, 0x38), shl(0x60, deployer))
-            mstore(add(ptr, 0x4c), salt)
-            mstore(add(ptr, 0x6c), keccak256(ptr, 0x37))
-            predicted := keccak256(add(ptr, 0x37), 0x55)
+            mstore(add(ptr, 0x38), deployer)
+            mstore(add(ptr, 0x24), 0x5af43d82803e903d91602b57fd5bf3ff)
+            mstore(add(ptr, 0x14), implementation)
+            mstore(ptr, 0x3d602d80600a3d3981f3363d3d373d3d3d363d73)
+            mstore(add(ptr, 0x58), salt)
+            mstore(add(ptr, 0x78), keccak256(add(ptr, 0x0c), 0x37))
+            predicted := keccak256(add(ptr, 0x43), 0x55)
         }
     }