refactor: add input validations

lukso-network · Aug 15, 2023 · 0047be6 · 0047be6
1 parent aaf594f
commit 0047be6
Show file tree

Hide file tree

Showing 2 changed files with 59 additions and 17 deletions.
diff --git a/contracts/LSP6KeyManager/LSP6Errors.sol b/contracts/LSP6KeyManager/LSP6Errors.sol
@@ -91,18 +91,6 @@ error InvalidERC725Function(bytes4 invalidFunction);
  */
 error InvalidEncodedAllowedCalls(bytes allowedCallsValue);
 
-/**
- * @notice Could not store `invalidValue` inside the `AddressPermissions[]` Array at index: `dataKey`.
- * @dev Reverts when trying to set a value that is not 20 bytes long (not an `address`) under the `AddressPermissions[index]` data key.
- *
- * @param dataKey The `AddressPermissions[index]` data key, that specify the index in the `AddressPermissions[]` array.
- * @param invalidValue The invalid value that was attempted to be set under `AddressPermissions[index]`.
- */
-error AddressPermissionArrayIndexValueNotAnAddress(
-    bytes32 dataKey,
-    bytes invalidValue
-);
-
 /**
  * @notice The address `from` is not authorised to set data, because it has no ERC725Y Data Key allowed.
  *
@@ -242,6 +230,23 @@ error RelayCallBeforeStartTime();
 error RelayCallExpired();
 
 /**
- * @dev reverts when the address of the Key Manager is being set as extensions for lsp20 functions
+ * @notice Key Manager cannot be used as an LSP17 extension for LSP20 functions.
+ *
+ * @dev Reverts when the address of the Key Manager is being set as extensions for lsp20 functions
  */
 error KeyManagerCannotBeSetAsExtensionForLSP20Functions();
+
+/**
+ * @notice Data value: `dataValue` length is different from `zeroValueRequiredLength` and `normalValueRequiredLength`.
+ *
+ * @dev Reverts when the data value length is not one of the required lengths for the specific data key.
+ *
+ * @param dataKey The data key associated with the invalid `dataValue` that was set.
+ * @param dataValue The data value that has an invalid length.
+ * @param requiredLength The `dataValue` length required for the zero value set under the `dataKey`.
+ */
+error InvalidDataValuesForDataKeys(
+    bytes32 dataKey,
+    bytes dataValue,
+    uint256 requiredLength
+);
diff --git a/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol b/contracts/LSP6KeyManager/LSP6Modules/LSP6SetDataModule.sol
@@ -43,13 +43,13 @@ import {
 // errors
 import {
     NotRecognisedPermissionKey,
-    AddressPermissionArrayIndexValueNotAnAddress,
     InvalidEncodedAllowedCalls,
     InvalidEncodedAllowedERC725YDataKeys,
     NoERC725YDataKeysAllowed,
     NotAllowedERC725YDataKey,
     NotAuthorised,
-    KeyManagerCannotBeSetAsExtensionForLSP20Functions
+    KeyManagerCannotBeSetAsExtensionForLSP20Functions,
+    InvalidDataValuesForDataKeys
 } from "../LSP6Errors.sol";
 
 abstract contract LSP6SetDataModule {
@@ -228,6 +228,15 @@ abstract contract LSP6SetDataModule {
                 bytes12(inputDataKey) ==
                 _LSP6KEY_ADDRESSPERMISSIONS_PERMISSIONS_PREFIX
             ) {
+                // validate the number of bytes in `dataValue`
+                if (inputDataValue.length != 32 && inputDataValue.length != 0) {
+                    revert InvalidDataValuesForDataKeys(
+                        inputDataKey,
+                        inputDataValue,
+                        32
+                    );
+                }
+
                 // controller already has the permissions needed. Do not run internal function.
                 if (hasBothAddControllerAndEditPermissions) return (bytes32(0));
 
@@ -286,6 +295,15 @@ abstract contract LSP6SetDataModule {
             inputDataKey == _LSP1_UNIVERSAL_RECEIVER_DELEGATE_KEY ||
             bytes12(inputDataKey) == _LSP1_UNIVERSAL_RECEIVER_DELEGATE_PREFIX
         ) {
+            // validate the number of bytes in `dataValue`
+            if (inputDataValue.length != 20 && inputDataValue.length != 0) {
+                revert InvalidDataValuesForDataKeys(
+                    inputDataKey,
+                    inputDataValue,
+                    20
+                );
+            }
+
             // same as above. If controller has both permissions, do not read the `target` storage
             // to save gas by avoiding an extra external `view` call.
             if (
@@ -305,6 +323,15 @@ abstract contract LSP6SetDataModule {
 
             // LSP17Extension:<bytes4>
         } else if (bytes12(inputDataKey) == _LSP17_EXTENSION_PREFIX) {
+            // validate the number of bytes in `dataValue`
+            if (inputDataValue.length != 20 && inputDataValue.length != 0) {
+                revert InvalidDataValuesForDataKeys(
+                    inputDataKey,
+                    inputDataValue,
+                    20
+                );
+            }
+
             // reverts when the address of the Key Manager is being set as extensions for lsp20 functions
             bytes4 selector = bytes4(inputDataKey << 96);
 
@@ -355,6 +382,15 @@ abstract contract LSP6SetDataModule {
     ) internal view virtual returns (bytes32) {
         // AddressPermissions[] -> array length
         if (inputDataKey == _LSP6KEY_ADDRESSPERMISSIONS_ARRAY) {
+            // validate the number of bytes in `dataValue`
+            if (inputDataValue.length != 16 && inputDataValue.length != 0) {
+                revert InvalidDataValuesForDataKeys(
+                    inputDataKey,
+                    inputDataValue,
+                    16
+                );
+            }
+
             // if the controller already has both permissions from one of the two required,
             // No permission required as CHECK is already done. We don't need to read `target` storage.
             if (hasBothAddControllerAndEditPermissions) return bytes32(0);
@@ -376,9 +412,10 @@ abstract contract LSP6SetDataModule {
 
         // CHECK that we either ADD an address (20 bytes long) or REMOVE an address (0x)
         if (inputDataValue.length != 0 && inputDataValue.length != 20) {
-            revert AddressPermissionArrayIndexValueNotAnAddress(
+            revert InvalidDataValuesForDataKeys(
                 inputDataKey,
-                inputDataValue
+                inputDataValue,
+                20
             );
         }