perf: replace calldata slices by abi.decode to extract ERC725X.execute params

contracts/LSP6KeyManager/LSP6Errors.sol

@@ -114,10 +114,9 @@ error AddressPermissionArrayIndexValueNotAnAddress(
 error NoERC725YDataKeysAllowed(address from);
 
 /**
- * @notice The address `from` is not authorised to use the linked account contract to make external calls, because it has Allowed Calls set.
+ * @notice The address `from` is not authorised to use the linked account contract to make external calls, because it has no Allowed Calls set.
  *
- * @dev Reverts when the `from` address has no `AllowedCalls` set and cannot interact with any address
- * using the linked {target}.
+ * @dev Reverts when the `from` address has no `AllowedCalls` set and cannot interact with any address using the linked {target}.
  *
  * @param from The address that has no AllowedCalls.
  */

contracts/LSP6KeyManager/LSP6KeyManagerCore.sol

@@ -153,7 +153,7 @@ abstract contract LSP6KeyManagerCore is
     /**
      * @inheritdoc ILSP6KeyManager
      *
-     * @custom:events VerifiedCall event when the permissions related to `payload` have been verified successfully.
+     * @custom:events {PermissionsVerified} event when the permissions related to `payload` have been verified successfully.
      */
     function execute(
         bytes calldata payload
@@ -164,7 +164,7 @@ abstract contract LSP6KeyManagerCore is
     /**
      * @inheritdoc ILSP6KeyManager
      *
-     * @custom:events VerifiedCall event for each permissions related to each `payload` that have been verified successfully.
+     * @custom:events {PermissionsVerified} event for each permissions related to each `payload` that have been verified successfully.
      */
     function executeBatch(
         uint256[] calldata values,
@@ -199,7 +199,7 @@ abstract contract LSP6KeyManagerCore is
     /**
      * @inheritdoc ILSP6KeyManager
      *
-     * @custom:events {VerifiedCall} event when the permissions related to `payload` have been verified successfully.
+     * @custom:events {PermissionsVerified} event when the permissions related to `payload` have been verified successfully.
      *
      * @custom:hint You can use `validityTimestamps == 0` to define an `executeRelayCall` transaction that is indefinitely valid,
      * meaning that does not require to start from a specific date/time, or that has an expiration date/time/
@@ -528,11 +528,21 @@ abstract contract LSP6KeyManagerCore is
 
             // ERC725X.execute(uint256,address,uint256,bytes)
         } else if (erc725Function == IERC725X.execute.selector) {
+            (
+                uint256 operationType,
+                address to,
+                uint256 value,
+                bytes memory data
+            ) = abi.decode(payload[4:], (uint256, address, uint256, bytes));
+
             LSP6ExecuteModule._verifyCanExecute(
                 _target,
                 from,
                 permissions,
-                payload
+                operationType,
+                to,
+                value,
+                data
             );
         } else if (
             erc725Function == ILSP14Ownable2Step.transferOwnership.selector ||

contracts/LSP6KeyManager/LSP6Modules/LSP6ExecuteModule.sol

@@ -54,32 +54,16 @@ abstract contract LSP6ExecuteModule {
      * @param controlledContract the address of the ERC725 contract where the payload is executed and where the permissions are verified.
      * @param controller the address who want to run the execute function on the ERC725Account.
      * @param permissions the permissions of the controller address.
-     * @param payload the ABI encoded payload `controlledContract.execute(...)`.
      */
     function _verifyCanExecute(
         address controlledContract,
         address controller,
         bytes32 permissions,
-        bytes calldata payload
+        uint256 operationType,
+        address to,
+        uint256 value,
+        bytes memory data
     ) internal view virtual {
-        // CHECK the offset of `data` is not pointing to the previous parameters
-        //
-        // offsets in calldata for ERC725X.execute(...) parameters (excluding function selector)
-        //
-        // - `operationType`: index 0 in calldata
-        // - `to`: index 32
-        // - `value`: index 64
-        // - `data`'s offset location: index 96
-        // - `data` starts at: index 128 (= 0x0000...0080)
-        if (bytes32(payload[100:132]) != bytes32(uint256(128))) {
-            revert InvalidPayload(payload);
-        }
-
-        // MUST be one of the ERC725X operation types.
-        uint256 operationType = uint256(bytes32(payload[4:36]));
-
-        address to = address(bytes20(payload[48:68]));
-
         // if to is the KeyManager address revert
         if (to == address(this)) {
             revert CallingKeyManagerNotAllowed();
@@ -92,10 +76,10 @@ abstract contract LSP6ExecuteModule {
         // Check to restrict controllers with execute permissions to call lsp20 functions
         // to avoid setting the reentrancy guard to a non-valid state
 
-        // if (payload.length >= 168 && to == address(this)) {
+        // if (data.length >= 4 && to == address(this)) {
         //     if (
-        //         bytes4(payload[164:168]) == ILSP20.lsp20VerifyCall.selector ||
-        //         bytes4(payload[164:168]) == ILSP20.lsp20VerifyCallResult.selector
+        //         bytes4(data) == ILSP20.lsp20VerifyCall.selector ||
+        //         bytes4(data) == ILSP20.lsp20VerifyCallResult.selector
         //     ) {
         //         revert CallingLSP20FunctionsOnLSP6NotAllowed();
         //     }
@@ -108,7 +92,9 @@ abstract contract LSP6ExecuteModule {
                     controlledContract,
                     controller,
                     permissions,
-                    payload
+                    to,
+                    value,
+                    data
                 );
         }
 
@@ -119,7 +105,7 @@ abstract contract LSP6ExecuteModule {
         ) {
             // required to check for permission TRANSFERVALUE if we are funding
             // the contract on deployment via a payable constructor
-            bool isFundingContract = uint256(bytes32(payload[68:100])) != 0;
+            bool isFundingContract = value != 0;
 
             return
                 _verifyCanDeployContract(
@@ -138,7 +124,9 @@ abstract contract LSP6ExecuteModule {
                     controlledContract,
                     controller,
                     permissions,
-                    payload
+                    to,
+                    value,
+                    data
                 );
         }
 
@@ -169,7 +157,9 @@ abstract contract LSP6ExecuteModule {
         address controlledContract,
         address controller,
         bytes32 permissions,
-        bytes calldata payload
+        address to,
+        uint256 value,
+        bytes memory data
     ) internal view virtual {
         bool hasSuperStaticCall = permissions.hasPermission(
             _PERMISSION_SUPER_STATICCALL
@@ -180,36 +170,35 @@ abstract contract LSP6ExecuteModule {
 
         _requirePermissions(controller, permissions, _PERMISSION_STATICCALL);
 
-        _verifyAllowedCall(controlledContract, controller, payload);
+        _verifyAllowedCall(
+            controlledContract,
+            controller,
+            OPERATION_3_STATICCALL,
+            to,
+            value,
+            data
+        );
     }
 
     function _verifyCanCall(
         address controlledContract,
         address controller,
         bytes32 permissions,
-        bytes calldata payload
+        address to,
+        uint256 value,
+        bytes memory data
     ) internal view virtual {
-        bool isTransferringValue = uint256(bytes32(payload[68:100])) != 0;
+        bool isValueTransfer = value != 0;
 
         bool hasSuperTransferValue = permissions.hasPermission(
             _PERMISSION_SUPER_TRANSFERVALUE
         );
 
-        // all the parameters are abi-encoded (padded to 32 bytes words)
-        //
-        //    4 (ERC725X.execute selector)
-        // + 32 (uint256 operationType)
-        // + 32 (address to/target)
-        // + 32 (uint256 value)
-        // + 32 (`data` offset)
-        // + 32 (`data` length)
-        // --------------------
-        // = 164 bytes in total
-        bool isCallDataPresent = payload.length > 164;
+        bool isEmptyCall = data.length == 0;
 
         bool hasSuperCall = permissions.hasPermission(_PERMISSION_SUPER_CALL);
 
-        if (isTransferringValue && !hasSuperTransferValue) {
+        if (isValueTransfer && !hasSuperTransferValue) {
             _requirePermissions(
                 controller,
                 permissions,
@@ -219,40 +208,41 @@ abstract contract LSP6ExecuteModule {
 
         // CHECK if we are doing an empty call, as the receive() or fallback() function
         // of the controlledContract could run some code.
-        if (!hasSuperCall && !isCallDataPresent && !isTransferringValue) {
+        if (isEmptyCall && !isValueTransfer && !hasSuperCall) {
             _requirePermissions(controller, permissions, _PERMISSION_CALL);
         }
 
-        if (isCallDataPresent && !hasSuperCall) {
+        if (!isEmptyCall && !hasSuperCall) {
             _requirePermissions(controller, permissions, _PERMISSION_CALL);
         }
 
         // Skip if caller has SUPER permissions for external calls, with or without calldata (empty calls)
-        if (hasSuperCall && !isTransferringValue) return;
+        if (!isValueTransfer && hasSuperCall) return;
 
         // Skip if caller has SUPER permission for value transfers
-        if (hasSuperTransferValue && !isCallDataPresent && isTransferringValue)
-            return;
+        if (isEmptyCall && isValueTransfer && hasSuperTransferValue) return;
 
         // Skip if both SUPER permissions are present
         if (hasSuperCall && hasSuperTransferValue) return;
 
-        _verifyAllowedCall(controlledContract, controller, payload);
+        _verifyAllowedCall(
+            controlledContract,
+            controller,
+            OPERATION_0_CALL,
+            to,
+            value,
+            data
+        );
     }
 
     function _verifyAllowedCall(
         address controlledContract,
         address controllerAddress,
-        bytes calldata payload
+        uint256 operationType,
+        address to,
+        uint256 value,
+        bytes memory data
     ) internal view virtual {
-        (
-            uint256 operationType,
-            address to,
-            uint256 value,
-            bytes4 selector,
-            bool isEmptyCall
-        ) = _extractExecuteParameters(payload);
-
         // CHECK for ALLOWED CALLS
         bytes memory allowedCalls = ERC725Y(controlledContract)
             .getAllowedCallsFor(controllerAddress);
@@ -261,6 +251,11 @@ abstract contract LSP6ExecuteModule {
             revert NoCallsAllowed(controllerAddress);
         }
 
+        bool isEmptyCall = data.length == 0;
+
+        // CHECK if there is at least a 4 bytes function selector
+        bytes4 selector = data.length >= 4 ? bytes4(data) : bytes4(0);
+
         bytes4 requiredCallTypes = _extractCallType(
             operationType,
             value,
@@ -334,33 +329,6 @@ abstract contract LSP6ExecuteModule {
         }
     }
 
-    function _extractExecuteParameters(
-        bytes calldata executeCalldata
-    ) internal pure returns (uint256, address, uint256, bytes4, bool) {
-        uint256 operationType = uint256(bytes32(executeCalldata[4:36]));
-
-        // CHECK that it is a valid address left-padded with `00` on the 12 upper bytes
-        if (bytes12(executeCalldata[36:48]) != bytes12(0)) {
-            revert InvalidPayload(executeCalldata);
-        }
-        address to = address(bytes20(executeCalldata[48:68]));
-
-        uint256 value = uint256(bytes32(executeCalldata[68:100]));
-
-        // CHECK if there is at least a 4 bytes function selector
-        bytes4 selector = executeCalldata.length >= 168
-            ? bytes4(executeCalldata[164:168])
-            : bytes4(0);
-
-        return (
-            operationType,
-            to,
-            value,
-            selector,
-            executeCalldata.length == 164
-        );
-    }
-
     function _isAllowedAddress(
         bytes memory allowedCall,
         address to

contracts/Mocks/KeyManager/KeyManagerInternalsTester.sol

@@ -34,9 +34,19 @@ contract KeyManagerInternalTester is LSP6KeyManager {
 
     function verifyAllowedCall(
         address _sender,
-        bytes calldata _payload
+        uint256 operationType,
+        address to,
+        uint256 value,
+        bytes memory data
     ) public view {
-        super._verifyAllowedCall(_target, _sender, _payload);
+        super._verifyAllowedCall(
+            _target,
+            _sender,
+            operationType,
+            to,
+            value,
+            data
+        );
     }
 
     function isCompactBytesArrayOfAllowedCalls(
@@ -103,9 +113,11 @@ contract KeyManagerInternalTester is LSP6KeyManager {
         return _addressPermission.hasPermission(_permissions);
     }
 
-    function extractExecuteParameters(
-        bytes calldata executeCalldata
-    ) public pure returns (uint256, address, uint256, bytes4, bool) {
-        return super._extractExecuteParameters(executeCalldata);
+    function verifyPermissions(
+        address from,
+        uint256 msgValue,
+        bytes calldata payload
+    ) public view {
+        super._verifyPermissions(from, msgValue, payload);
     }
 }

contracts/Mocks/TargetContract.sol

@@ -27,6 +27,10 @@ contract TargetContract {
     }
 
     function setNamePayable(string memory name) public payable {
+        _name = name;
+    }
+
+    function setNamePayableMinimumValue(string memory name) public payable {
         require(msg.value >= 50, "Not enough value provided");
         _name = name;
     }