refactor: use uint256 for reentrancyStatus

lukso-network · Oct 2, 2023 · 685872c · 685872c
1 parent 91fcc47
commit 685872c
Show file tree

Hide file tree

Showing 5 changed files with 32 additions and 26 deletions.
diff --git a/constants.ts b/constants.ts
@@ -60,10 +60,10 @@ export const ERC1271_VALUES = {
  */
 export const LSP20_MAGIC_VALUES = {
   VERIFY_CALL: {
-    // bytes3(keccak256("lsp20VerifyCall(address,uint256,bytes)")) + "0x01"
-    WITH_POST_VERIFICATION: '0x1a238001',
-    // bytes3(keccak256("lsp20VerifyCall(address,uint256,bytes)")) + "0x00"
+    // bytes3(keccak256("lsp20VerifyCall(address,address,uint256,bytes)")) + "0x00"
     NO_POST_VERIFICATION: '0x1a238000',
+    // bytes3(keccak256("lsp20VerifyCall(address,address,uint256,bytes)")) + "0x01"
+    WITH_POST_VERIFICATION: '0x1a238001',
   },
   // bytes4(keccak256("lsp20VerifyCallResult(bytes32,bytes)"))
   VERIFY_CALL_RESULT: '0xd3fc45d3',

diff --git a/contracts/LSP20CallVerification/ILSP20CallVerifier.sol b/contracts/LSP20CallVerification/ILSP20CallVerifier.sol
@@ -12,7 +12,7 @@ interface ILSP20CallVerifier {
      * the function is allowed, concatened with a byte that determines if the lsp20VerifyCallResult function should
      * be called after the original function call. The byte that invoke the lsp20VerifyCallResult function is strictly `0x01`.
      *
-     * @param callee The address of the contract that implements the LSP20 interface
+     * @param callee The address of the contract that implements the `LSP20CallVerifier` interface
      * @param caller The address who called the function on the msg.sender
      * @param value The value sent by the caller to the function called on the msg.sender
      * @param receivedCalldata The receivedCalldata sent by the caller to the msg.sender

diff --git a/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol b/contracts/LSP6KeyManager/LSP6KeyManagerCore.sol
@@ -98,9 +98,9 @@ abstract contract LSP6KeyManagerCore is
 
     // Variables, methods and modifier used for ReentrancyGuard are taken from the link below and modified accordingly.
     // https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v4.8/contracts/security/ReentrancyGuard.sol
-    mapping(address => uint8) internal _reentrancyStatus;
-    uint8 internal constant _NOT_ENTERED = 1;
-    uint8 internal constant _ENTERED = 2;
+    mapping(address => uint256) internal _reentrancyStatus;
+    uint256 internal constant _NOT_ENTERED = 1;
+    uint256 internal constant _ENTERED = 2;
 
     /**
      * @inheritdoc ILSP6
@@ -333,7 +333,7 @@ abstract contract LSP6KeyManagerCore is
 
         // If target is invoking the verification, emit the event and change the reentrancy guard
         if (msg.sender == targetContract) {
-            uint8 reentrancyStatus = _nonReentrantBefore(
+            uint256 reentrancyStatus = _nonReentrantBefore(
                 targetContract,
                 isSetData,
                 caller
@@ -351,7 +351,7 @@ abstract contract LSP6KeyManagerCore is
         /// @dev If a different address is invoking the verification,
         /// do not change the state or emit the event to allow read-only verification
         else {
-            uint8 reentrancyStatus = _reentrancyStatus[targetContract];
+            uint256 reentrancyStatus = _reentrancyStatus[targetContract];
 
             if (reentrancyStatus == _ENTERED) {
                 _requirePermissions(
@@ -399,7 +399,7 @@ abstract contract LSP6KeyManagerCore is
 
         address targetContract = _target;
 
-        uint8 reentrancyStatus = _nonReentrantBefore(
+        uint256 reentrancyStatus = _nonReentrantBefore(
             targetContract,
             isSetData,
             msg.sender
@@ -476,7 +476,7 @@ abstract contract LSP6KeyManagerCore is
         bool isSetData = bytes4(payload) == IERC725Y.setData.selector ||
             bytes4(payload) == IERC725Y.setDataBatch.selector;
 
-        uint8 reentrancyStatus = _nonReentrantBefore(
+        uint256 reentrancyStatus = _nonReentrantBefore(
             targetContract,
             isSetData,
             signer
@@ -536,6 +536,13 @@ abstract contract LSP6KeyManagerCore is
         bytes32 permissions = ERC725Y(targetContract).getPermissionsFor(from);
         if (permissions == bytes32(0)) revert NoPermissionsSet(from);
 
+        if (isRelayedCall) {
+            LSP6ExecuteRelayCallModule._verifyExecuteRelayCallPermission(
+                from,
+                permissions
+            );
+        }
+
         bytes4 erc725Function = bytes4(payload);
 
         // ERC725Y.setData(bytes32,bytes)
@@ -595,13 +602,6 @@ abstract contract LSP6KeyManagerCore is
         } else {
             revert InvalidERC725Function(erc725Function);
         }
-
-        if (isRelayedCall) {
-            LSP6ExecuteRelayCallModule._verifyExecuteRelayCallPermission(
-                from,
-                permissions
-            );
-        }
     }
 
     /**
@@ -622,7 +622,7 @@ abstract contract LSP6KeyManagerCore is
         address targetContract,
         bool isSetData,
         address from
-    ) internal virtual returns (uint8 reentrancyStatus) {
+    ) internal virtual returns (uint256 reentrancyStatus) {
         reentrancyStatus = _reentrancyStatus[targetContract];
         if (reentrancyStatus == _ENTERED) {
             // CHECK the caller has REENTRANCY permission

diff --git a/docs/contracts/LSP6KeyManager/LSP6KeyManager.md b/docs/contracts/LSP6KeyManager/LSP6KeyManager.md
@@ -1204,7 +1204,7 @@ function _nonReentrantBefore(
   address targetContract,
   bool isSetData,
   address from
-) internal nonpayable returns (uint8 reentrancyStatus);
+) internal nonpayable returns (uint256 reentrancyStatus);
 ```
 
 Update the status from `_NON_ENTERED` to `_ENTERED` and checks if

diff --git a/tests/Reentrancy/LSP20/reentrancyHelpers.ts b/tests/Reentrancy/LSP20/reentrancyHelpers.ts
@@ -261,7 +261,7 @@ export const addUniversalReceiverDelegateTestCases = {
   NotAuthorised: [
     {
       permissionsText: 'NO Permissions',
-      permissions: '0x',
+      permissions: PERMISSIONS.EXECUTE_RELAY_CALL,
       missingPermission: 'REENTRANCY',
     },
     {
@@ -271,12 +271,15 @@ export const addUniversalReceiverDelegateTestCases = {
     },
     {
       permissionsText: 'REENTRANCY',
-      permissions: PERMISSIONS.REENTRANCY,
+      permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY),
       missingPermission: 'ADDUNIVERSALRECEIVERDELEGATE',
     },
     {
       permissionsText: 'ADDUNIVERSALRECEIVERDELEGATE',
-      permissions: PERMISSIONS.ADDUNIVERSALRECEIVERDELEGATE,
+      permissions: combinePermissions(
+        PERMISSIONS.EXECUTE_RELAY_CALL,
+        PERMISSIONS.ADDUNIVERSALRECEIVERDELEGATE,
+      ),
       missingPermission: 'REENTRANCY',
     },
   ],
@@ -295,7 +298,7 @@ export const changeUniversalReceiverDelegateTestCases = {
   NotAuthorised: [
     {
       permissionsText: 'NO Permissions',
-      permissions: '0x',
+      permissions: PERMISSIONS.EXECUTE_RELAY_CALL,
       missingPermission: 'REENTRANCY',
     },
     {
@@ -305,12 +308,15 @@ export const changeUniversalReceiverDelegateTestCases = {
     },
     {
       permissionsText: 'REENTRANCY',
-      permissions: PERMISSIONS.REENTRANCY,
+      permissions: combinePermissions(PERMISSIONS.EXECUTE_RELAY_CALL, PERMISSIONS.REENTRANCY),
       missingPermission: 'CHANGEUNIVERSALRECEIVERDELEGATE',
     },
     {
       permissionsText: 'CHANGEUNIVERSALRECEIVERDELEGATE',
-      permissions: PERMISSIONS.CHANGEUNIVERSALRECEIVERDELEGATE,
+      permissions: combinePermissions(
+        PERMISSIONS.EXECUTE_RELAY_CALL,
+        PERMISSIONS.CHANGEUNIVERSALRECEIVERDELEGATE,
+      ),
       missingPermission: 'REENTRANCY',
     },
   ],