Expand malicious module test for all module types (#96)

* add notes * add placeholders * remove selfdestruct test, it simply doesnt work due to end-of-transaction behavior of selfdestruct and the effective noop in Foundry, add check inside of endpoint tests for deprecated module reversion for completeness * simplify external target * clean up * comment out note * clean up * complete malicious layout change check, behaviour is as expected across all module types * clean up, fix lint issue
chromaxyz · Jun 21, 2023 · 367605f · 367605f
1 parent 5d953f3
commit 367605f
Show file tree

Hide file tree

Showing 14 changed files with 387 additions and 317 deletions.
diff --git a/reports/SLITHER_CHECKLIST.md b/reports/SLITHER_CHECKLIST.md
@@ -290,9 +290,9 @@ Confidence: High
 ../src/ReflexModule.sol#L39
 
 - [ ] ID-49
-      Variable [ReflexState.\_\_REFLEX_GAP](../src/ReflexState.sol#L90) is not in mixedCase
+      Variable [ReflexState.\_\_REFLEX_GAP](../src/ReflexState.sol#L91) is not in mixedCase
 
-../src/ReflexState.sol#L90
+../src/ReflexState.sol#L91
 
 - [ ] ID-50
       Variable [ReflexState.\_reentrancyStatus](../src/ReflexState.sol#L35) is not in mixedCase

diff --git a/src/ReflexState.sol b/src/ReflexState.sol
@@ -87,5 +87,6 @@ abstract contract ReflexState is IReflexState, ReflexConstants {
      * @dev Reflex occupies storage slots 0 to 49 (50 in total).
      * @dev Storage slot: 6 (1408 bytes).
      */
+    // solhint-disable-next-line var-name-mixedcase
     uint256[44] private __REFLEX_GAP;
 }
diff --git a/src/periphery/ReflexBatch.sol b/src/periphery/ReflexBatch.sol
@@ -91,7 +91,7 @@ abstract contract ReflexBatch is IReflexBatch, ReflexModule {
     function simulateBatchCallReturn(
         BatchAction[] calldata actions_
     ) public virtual reentrancyAllowed returns (BatchActionResponse[] memory simulation_) {
-        // NOTE: it is assumed user will never be able to control _modules (storage) nor _moduleId (immutable).
+        // NOTE: it is assumed attacker will never be able to control _modules (storage) nor _moduleId (immutable).
         // TODO: gas optimization: `_unpackEndpointAddress` could be replaced by msg.sender.
         // TODO: gas optimization: `success` check is strictly not necessary and may interfere.
 

diff --git a/test/ImplementationERC20.t.sol b/test/ImplementationERC20.t.sol
@@ -2,7 +2,6 @@
 pragma solidity ^0.8.13;
 
 // Vendor
-// solhint-disable-next-line no-console
 import {stdError} from "forge-std/StdError.sol";
 
 // Interfaces

diff --git a/test/ImplementationModuleInternal.t.sol b/test/ImplementationModuleInternal.t.sol
@@ -2,14 +2,15 @@
 pragma solidity ^0.8.13;
 
 // Interfaces
+import {IReflexInstaller} from "../src/interfaces/IReflexInstaller.sol";
 import {IReflexModule} from "../src/interfaces/IReflexModule.sol";
 
 // Fixtures
 import {ImplementationFixture} from "./fixtures/ImplementationFixture.sol";
 
 // Mocks
 import {MockImplementationDeprecatedModule} from "./mocks/MockImplementationDeprecatedModule.sol";
-import {MockImplementationMaliciousModule} from "./mocks/MockImplementationMaliciousModule.sol";
+import {MockImplementationMaliciousStorageModule} from "./mocks/MockImplementationMaliciousStorageModule.sol";
 import {MockImplementationModule} from "./mocks/MockImplementationModule.sol";
 
 /**
@@ -49,7 +50,8 @@ contract ImplementationModuleInternalTest is ImplementationFixture {
     MockImplementationModule public internalModuleV1;
     MockImplementationModule public internalModuleV2;
     MockImplementationDeprecatedModule public internalModuleV3;
-    MockImplementationMaliciousModule public internalModuleV4;
+    MockImplementationModule public internalModuleV4;
+    MockImplementationMaliciousStorageModule public internalModuleMaliciousStorageV4;
 
     // =====
     // Setup
@@ -103,7 +105,16 @@ contract ImplementationModuleInternalTest is ImplementationFixture {
             })
         );
 
-        internalModuleV4 = new MockImplementationMaliciousModule(
+        internalModuleV4 = new MockImplementationModule(
+            IReflexModule.ModuleSettings({
+                moduleId: _MODULE_INTERNAL_ID,
+                moduleType: _MODULE_INTERNAL_TYPE,
+                moduleVersion: _MODULE_INTERNAL_VERSION_V4,
+                moduleUpgradeable: _MODULE_INTERNAL_UPGRADEABLE_V4
+            })
+        );
+
+        internalModuleMaliciousStorageV4 = new MockImplementationMaliciousStorageModule(
             IReflexModule.ModuleSettings({
                 moduleId: _MODULE_INTERNAL_ID,
                 moduleType: _MODULE_INTERNAL_TYPE,
@@ -192,6 +203,14 @@ contract ImplementationModuleInternalTest is ImplementationFixture {
             _MODULE_INTERNAL_VERSION_V4,
             _MODULE_INTERNAL_UPGRADEABLE_V4
         );
+
+        _verifyModuleConfiguration(
+            internalModuleMaliciousStorageV4,
+            _MODULE_INTERNAL_ID,
+            _MODULE_INTERNAL_TYPE,
+            _MODULE_INTERNAL_VERSION_V4,
+            _MODULE_INTERNAL_UPGRADEABLE_V4
+        );
     }
 
     function testFuzzCallInternalModule(bytes32 message_) external {
@@ -236,14 +255,6 @@ contract ImplementationModuleInternalTest is ImplementationFixture {
 
         // Verify internal module.
 
-        _verifyModuleConfiguration(
-            singleModuleEndpoint,
-            _MODULE_SINGLE_ID,
-            _MODULE_SINGLE_TYPE,
-            _MODULE_SINGLE_VERSION_V1,
-            _MODULE_SINGLE_UPGRADEABLE_V1
-        );
-
         _verifyModuleConfiguration(
             IReflexModule.ModuleSettings({
                 moduleId: _MODULE_INTERNAL_ID,
@@ -308,9 +319,37 @@ contract ImplementationModuleInternalTest is ImplementationFixture {
         // Verify storage is not modified by upgrades in `Dispatcher` context.
 
         _verifyGetStateSlot(message_);
+
+        // Attempt to upgrade internal module that was marked as deprecated, this should fail.
+
+        moduleAddresses = new address[](1);
+        moduleAddresses[0] = address(internalModuleV4);
+
+        vm.expectRevert(
+            abi.encodeWithSelector(IReflexInstaller.ModuleNotUpgradeable.selector, internalModuleV4.moduleId())
+        );
+        installerEndpoint.upgradeModules(moduleAddresses);
+
+        // Verify internal module was not upgraded.
+
+        _verifyModuleConfiguration(
+            IReflexModule.ModuleSettings({
+                moduleId: _MODULE_INTERNAL_ID,
+                moduleType: _MODULE_INTERNAL_TYPE,
+                moduleVersion: _MODULE_INTERNAL_VERSION_V3,
+                moduleUpgradeable: _MODULE_INTERNAL_UPGRADEABLE_V3
+            })
+        );
+
+        // Verify storage is not modified by upgrades in `Dispatcher` context.
+
+        _verifyGetStateSlot(message_);
     }
 
-    function testFuzzUpgradeInternalModuleToMaliciousModule(bytes32 message_, uint8 number_) external brutalizeMemory {
+    function testFuzzUpgradeInternalModuleToMaliciousStorageModule(
+        bytes32 message_,
+        uint8 number_
+    ) external brutalizeMemory {
         vm.assume(uint8(uint256(message_)) != number_);
 
         // Verify storage sets in `Dispatcher` context.
@@ -319,14 +358,6 @@ contract ImplementationModuleInternalTest is ImplementationFixture {
 
         assertEq(dispatcher.getImplementationState0(), message_);
 
-        _verifyModuleConfiguration(
-            singleModuleEndpoint,
-            _MODULE_SINGLE_ID,
-            _MODULE_SINGLE_TYPE,
-            _MODULE_SINGLE_VERSION_V1,
-            _MODULE_SINGLE_UPGRADEABLE_V1
-        );
-
         _verifyModuleConfiguration(
             IReflexModule.ModuleSettings({
                 moduleId: _MODULE_INTERNAL_ID,
@@ -337,7 +368,7 @@ contract ImplementationModuleInternalTest is ImplementationFixture {
         );
 
         address[] memory moduleAddresses = new address[](1);
-        moduleAddresses[0] = address(internalModuleV4);
+        moduleAddresses[0] = address(internalModuleMaliciousStorageV4);
         installerEndpoint.upgradeModules(moduleAddresses);
 
         _verifyModuleConfiguration(
@@ -366,7 +397,7 @@ contract ImplementationModuleInternalTest is ImplementationFixture {
             number_
         );
 
-        // Verify that the storage in the `Dispatcher` context has been overwritten.
+        // Verify that the storage in the `Dispatcher` context has been overwritten, this is disastrous.
 
         assertEq(uint8(uint256(dispatcher.getImplementationState0())), number_);
         assertFalse(dispatcher.getImplementationState0() == message_);