From 367605ff7a81a1fb101090b3bc796fa8e215eac5 Mon Sep 17 00:00:00 2001 From: zerosnacks <95942363+zerosnacks@users.noreply.github.com> Date: Wed, 21 Jun 2023 17:23:44 +0200 Subject: [PATCH] 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 --- reports/SLITHER_CHECKLIST.md | 4 +- src/ReflexState.sol | 1 + src/periphery/ReflexBatch.sol | 2 +- test/ImplementationERC20.t.sol | 1 - test/ImplementationModuleInternal.t.sol | 75 +++++-- test/ImplementationModuleMultiEndpoint.t.sol | 158 +++++++++++++- test/ImplementationModuleSingleEndpoint.t.sol | 117 ++++++++++- test/ImplementationState.t.sol | 6 +- test/ReflexBatch.t.sol | 74 +++++-- test/mocks/MockERC20.sol | 39 ---- ...kImplementationMaliciousStorageModule.sol} | 4 +- test/mocks/MockImplementationState.sol | 22 +- test/mocks/MockReflexBase.sol | 3 + test/mocks/abstracts/ExternalERC20.sol | 198 ------------------ 14 files changed, 387 insertions(+), 317 deletions(-) delete mode 100644 test/mocks/MockERC20.sol rename test/mocks/{MockImplementationMaliciousModule.sol => MockImplementationMaliciousStorageModule.sol} (85%) delete mode 100644 test/mocks/abstracts/ExternalERC20.sol diff --git a/reports/SLITHER_CHECKLIST.md b/reports/SLITHER_CHECKLIST.md index 53db73f..70360cd 100644 --- 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 index 75f985f..566ce13 100644 --- 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 index ae311dd..50258d5 100644 --- 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 index de8b2d4..746c2d1 100644 --- 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 index 65dc25f..ff3819c 100644 --- a/test/ImplementationModuleInternal.t.sol +++ b/test/ImplementationModuleInternal.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.13; // Interfaces +import {IReflexInstaller} from "../src/interfaces/IReflexInstaller.sol"; import {IReflexModule} from "../src/interfaces/IReflexModule.sol"; // Fixtures @@ -9,7 +10,7 @@ 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_); diff --git a/test/ImplementationModuleMultiEndpoint.t.sol b/test/ImplementationModuleMultiEndpoint.t.sol index ed5794f..de0614b 100644 --- a/test/ImplementationModuleMultiEndpoint.t.sol +++ b/test/ImplementationModuleMultiEndpoint.t.sol @@ -2,8 +2,9 @@ pragma solidity ^0.8.13; // Interfaces -import {IReflexModule} from "../src/interfaces/IReflexModule.sol"; import {IReflexEndpoint} from "../src/interfaces/IReflexEndpoint.sol"; +import {IReflexInstaller} from "../src/interfaces/IReflexInstaller.sol"; +import {IReflexModule} from "../src/interfaces/IReflexModule.sol"; // Fixtures import {ImplementationFixture} from "./fixtures/ImplementationFixture.sol"; @@ -12,6 +13,7 @@ import {ImplementationFixture} from "./fixtures/ImplementationFixture.sol"; import {MockImplementationDeprecatedModule} from "./mocks/MockImplementationDeprecatedModule.sol"; import {MockImplementationERC20} from "./mocks/MockImplementationERC20.sol"; import {MockImplementationERC20Hub} from "./mocks/MockImplementationERC20Hub.sol"; +import {MockImplementationMaliciousStorageModule} from "./mocks/MockImplementationMaliciousStorageModule.sol"; /** * @title Implementation Module Multi Endpoint Test @@ -33,9 +35,11 @@ contract ImplementationModuleMultiEndpointTest is ImplementationFixture { uint16 internal constant _MODULE_MULTI_VERSION_V1 = 1; uint16 internal constant _MODULE_MULTI_VERSION_V2 = 2; uint16 internal constant _MODULE_MULTI_VERSION_V3 = 3; + uint16 internal constant _MODULE_MULTI_VERSION_V4 = 4; bool internal constant _MODULE_MULTI_UPGRADEABLE_V1 = true; bool internal constant _MODULE_MULTI_UPGRADEABLE_V2 = true; bool internal constant _MODULE_MULTI_UPGRADEABLE_V3 = false; + bool internal constant _MODULE_MULTI_UPGRADEABLE_V4 = true; string internal constant _MODULE_MULTI_NAME_A = "TOKEN A"; string internal constant _MODULE_MULTI_SYMBOL_A = "TKNA"; @@ -61,7 +65,9 @@ contract ImplementationModuleMultiEndpointTest is ImplementationFixture { MockImplementationERC20 public multiModuleV1; MockImplementationERC20 public multiModuleV2; - MockImplementationDeprecatedModule public multiModuleV3; + MockImplementationDeprecatedModule public multiModuleDeprecatedV3; + MockImplementationMaliciousStorageModule public multiModuleMaliciousStorageV3; + MockImplementationERC20 public multiModuleV4; MockImplementationERC20 public multiModuleEndpointA; MockImplementationERC20 public multiModuleEndpointB; @@ -110,7 +116,16 @@ contract ImplementationModuleMultiEndpointTest is ImplementationFixture { }) ); - multiModuleV3 = new MockImplementationDeprecatedModule( + multiModuleDeprecatedV3 = new MockImplementationDeprecatedModule( + IReflexModule.ModuleSettings({ + moduleId: _MODULE_MULTI_ID, + moduleType: _MODULE_MULTI_TYPE, + moduleVersion: _MODULE_MULTI_VERSION_V3, + moduleUpgradeable: _MODULE_MULTI_UPGRADEABLE_V3 + }) + ); + + multiModuleMaliciousStorageV3 = new MockImplementationMaliciousStorageModule( IReflexModule.ModuleSettings({ moduleId: _MODULE_MULTI_ID, moduleType: _MODULE_MULTI_TYPE, @@ -119,6 +134,15 @@ contract ImplementationModuleMultiEndpointTest is ImplementationFixture { }) ); + multiModuleV4 = new MockImplementationERC20( + IReflexModule.ModuleSettings({ + moduleId: _MODULE_MULTI_ID, + moduleType: _MODULE_MULTI_TYPE, + moduleVersion: _MODULE_MULTI_VERSION_V4, + moduleUpgradeable: _MODULE_MULTI_UPGRADEABLE_V4 + }) + ); + address[] memory moduleAddresses = new address[](2); moduleAddresses[0] = address(singleModuleV1); moduleAddresses[1] = address(multiModuleV1); @@ -242,12 +266,28 @@ contract ImplementationModuleMultiEndpointTest is ImplementationFixture { ); _verifyModuleConfiguration( - multiModuleV3, + multiModuleDeprecatedV3, + _MODULE_MULTI_ID, + _MODULE_MULTI_TYPE, + _MODULE_MULTI_VERSION_V3, + _MODULE_MULTI_UPGRADEABLE_V3 + ); + + _verifyModuleConfiguration( + multiModuleMaliciousStorageV3, _MODULE_MULTI_ID, _MODULE_MULTI_TYPE, _MODULE_MULTI_VERSION_V3, _MODULE_MULTI_UPGRADEABLE_V3 ); + + _verifyModuleConfiguration( + multiModuleV4, + _MODULE_MULTI_ID, + _MODULE_MULTI_TYPE, + _MODULE_MULTI_VERSION_V4, + _MODULE_MULTI_UPGRADEABLE_V4 + ); } function testFuzzUpgradeMultiEndpointAndDeprecate(bytes32 message_) external brutalizeMemory { @@ -336,7 +376,7 @@ contract ImplementationModuleMultiEndpointTest is ImplementationFixture { // Upgrade to deprecate multi-endpoint module. moduleAddresses = new address[](1); - moduleAddresses[0] = address(multiModuleV3); + moduleAddresses[0] = address(multiModuleDeprecatedV3); installerEndpoint.upgradeModules(moduleAddresses); _verifyModuleConfiguration( @@ -366,6 +406,111 @@ contract ImplementationModuleMultiEndpointTest is ImplementationFixture { // Verify storage is not modified by upgrades in `Dispatcher` context. _verifyGetStateSlot(message_); + + // Attempt to upgrade multi-endpoint module that was marked as deprecated, this should fail. + + moduleAddresses = new address[](1); + moduleAddresses[0] = address(multiModuleV4); + + vm.expectRevert( + abi.encodeWithSelector(IReflexInstaller.ModuleNotUpgradeable.selector, multiModuleV4.moduleId()) + ); + installerEndpoint.upgradeModules(moduleAddresses); + + // Verify multi-endpoint module was not upgraded. + + _verifyModuleConfiguration( + multiModuleEndpointA, + _MODULE_MULTI_ID, + _MODULE_MULTI_TYPE, + _MODULE_MULTI_VERSION_V3, + _MODULE_MULTI_UPGRADEABLE_V3 + ); + + _verifyModuleConfiguration( + multiModuleEndpointB, + _MODULE_MULTI_ID, + _MODULE_MULTI_TYPE, + _MODULE_MULTI_VERSION_V3, + _MODULE_MULTI_UPGRADEABLE_V3 + ); + + _verifyModuleConfiguration( + multiModuleEndpointC, + _MODULE_MULTI_ID, + _MODULE_MULTI_TYPE, + _MODULE_MULTI_VERSION_V3, + _MODULE_MULTI_UPGRADEABLE_V3 + ); + + // Verify storage is not modified by upgrades in `Dispatcher` context. + + _verifyGetStateSlot(message_); + } + + function testFuzzUpgradeMultiEndpointToMaliciousStorageModule( + bytes32 message_, + uint8 number_ + ) external brutalizeMemory { + vm.assume(uint8(uint256(message_)) != number_); + + // Verify storage sets in `Dispatcher` context. + + _verifySetStateSlot(message_); + + // Upgrade multi-endpoint module to malicious storage module. + + address[] memory moduleAddresses = new address[](1); + moduleAddresses[0] = address(multiModuleMaliciousStorageV3); + installerEndpoint.upgradeModules(moduleAddresses); + + _verifyModuleConfiguration( + multiModuleEndpointA, + _MODULE_MULTI_ID, + _MODULE_MULTI_TYPE, + _MODULE_MULTI_VERSION_V3, + _MODULE_MULTI_UPGRADEABLE_V3 + ); + + _verifyModuleConfiguration( + multiModuleEndpointB, + _MODULE_MULTI_ID, + _MODULE_MULTI_TYPE, + _MODULE_MULTI_VERSION_V3, + _MODULE_MULTI_UPGRADEABLE_V3 + ); + + _verifyModuleConfiguration( + multiModuleEndpointC, + _MODULE_MULTI_ID, + _MODULE_MULTI_TYPE, + _MODULE_MULTI_VERSION_V3, + _MODULE_MULTI_UPGRADEABLE_V3 + ); + + // Overwrite storage in the `Dispatcher` context from the malicious module. + + MockImplementationMaliciousStorageModule(address(multiModuleEndpointA)).setNumber(number_); + + // Verify storage has been modified by malicious upgrade in `Dispatcher` context. + + assertEq(MockImplementationMaliciousStorageModule(address(multiModuleEndpointA)).getNumber(), number_); + assertEq(MockImplementationMaliciousStorageModule(address(multiModuleEndpointB)).getNumber(), number_); + assertEq(MockImplementationMaliciousStorageModule(address(multiModuleEndpointC)).getNumber(), number_); + + // Verify that the storage in the `Dispatcher` context has been overwritten, this is disastrous. + + assertEq(uint8(uint256(dispatcher.getImplementationState0())), number_); + assertFalse(dispatcher.getImplementationState0() == message_); + + // Overwrite storage in the `Dispatcher` context. + + dispatcher.setImplementationState0(message_); + + // Verify that the storage in the `Dispatcher` context has been overwritten. + + assertEq(dispatcher.getImplementationState0(), message_); + assertFalse(uint8(uint256(dispatcher.getImplementationState0())) == number_); } function testUnitEndpointSentinelFallback() external { @@ -473,7 +618,8 @@ contract ImplementationModuleMultiEndpointTest is ImplementationFixture { assertEq(multiModuleV1.getImplementationState0(), 0); assertEq(multiModuleV2.getImplementationState0(), 0); - assertEq(multiModuleV3.getImplementationState0(), 0); + assertEq(multiModuleDeprecatedV3.getImplementationState0(), 0); + assertEq(multiModuleV4.getImplementationState0(), 0); assertEq(multiModuleEndpointA.getImplementationState0(), message_); assertEq(multiModuleEndpointB.getImplementationState0(), message_); diff --git a/test/ImplementationModuleSingleEndpoint.t.sol b/test/ImplementationModuleSingleEndpoint.t.sol index 72a8c49..62a6980 100644 --- a/test/ImplementationModuleSingleEndpoint.t.sol +++ b/test/ImplementationModuleSingleEndpoint.t.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.13; // Interfaces +import {IReflexInstaller} from "../src/interfaces/IReflexInstaller.sol"; import {IReflexModule} from "../src/interfaces/IReflexModule.sol"; import {IReflexEndpoint} from "../src/interfaces/IReflexEndpoint.sol"; @@ -10,7 +11,7 @@ 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"; /** @@ -26,11 +27,11 @@ contract ImplementationModuleSingleEndpointTest is ImplementationFixture { uint16 internal constant _MODULE_SINGLE_VERSION_V1 = 1; uint16 internal constant _MODULE_SINGLE_VERSION_V2 = 2; uint16 internal constant _MODULE_SINGLE_VERSION_V3 = 3; - uint16 internal constant _MODULE_SINGLE_VERSION_V4 = 3; + uint16 internal constant _MODULE_SINGLE_VERSION_V4 = 4; bool internal constant _MODULE_SINGLE_UPGRADEABLE_V1 = true; bool internal constant _MODULE_SINGLE_UPGRADEABLE_V2 = true; bool internal constant _MODULE_SINGLE_UPGRADEABLE_V3 = false; - bool internal constant _MODULE_SINGLE_UPGRADEABLE_V4 = false; + bool internal constant _MODULE_SINGLE_UPGRADEABLE_V4 = true; // ======= // Storage @@ -38,8 +39,10 @@ contract ImplementationModuleSingleEndpointTest is ImplementationFixture { MockImplementationModule public singleModuleV1; MockImplementationModule public singleModuleV2; - MockImplementationDeprecatedModule public singleModuleV3; - MockImplementationMaliciousModule public singleModuleV4; + MockImplementationDeprecatedModule public singleModuleDeprecatedV3; + MockImplementationMaliciousStorageModule public singleModuleMaliciousStorageV3; + MockImplementationModule public singleModuleV4; + MockImplementationModule public singleModuleEndpoint; // ===== @@ -67,7 +70,16 @@ contract ImplementationModuleSingleEndpointTest is ImplementationFixture { }) ); - singleModuleV3 = new MockImplementationDeprecatedModule( + singleModuleDeprecatedV3 = new MockImplementationDeprecatedModule( + IReflexModule.ModuleSettings({ + moduleId: _MODULE_SINGLE_ID, + moduleType: _MODULE_SINGLE_TYPE, + moduleVersion: _MODULE_SINGLE_VERSION_V3, + moduleUpgradeable: _MODULE_SINGLE_UPGRADEABLE_V3 + }) + ); + + singleModuleMaliciousStorageV3 = new MockImplementationMaliciousStorageModule( IReflexModule.ModuleSettings({ moduleId: _MODULE_SINGLE_ID, moduleType: _MODULE_SINGLE_TYPE, @@ -76,7 +88,7 @@ contract ImplementationModuleSingleEndpointTest is ImplementationFixture { }) ); - singleModuleV4 = new MockImplementationMaliciousModule( + singleModuleV4 = new MockImplementationModule( IReflexModule.ModuleSettings({ moduleId: _MODULE_SINGLE_ID, moduleType: _MODULE_SINGLE_TYPE, @@ -135,7 +147,15 @@ contract ImplementationModuleSingleEndpointTest is ImplementationFixture { ); _verifyModuleConfiguration( - singleModuleV3, + singleModuleDeprecatedV3, + _MODULE_SINGLE_ID, + _MODULE_SINGLE_TYPE, + _MODULE_SINGLE_VERSION_V3, + _MODULE_SINGLE_UPGRADEABLE_V3 + ); + + _verifyModuleConfiguration( + singleModuleMaliciousStorageV3, _MODULE_SINGLE_ID, _MODULE_SINGLE_TYPE, _MODULE_SINGLE_VERSION_V3, @@ -187,7 +207,7 @@ contract ImplementationModuleSingleEndpointTest is ImplementationFixture { // Upgrade to deprecate single-endpoint module. moduleAddresses = new address[](1); - moduleAddresses[0] = address(singleModuleV3); + moduleAddresses[0] = address(singleModuleDeprecatedV3); installerEndpoint.upgradeModules(moduleAddresses); _verifyModuleConfiguration( @@ -201,6 +221,85 @@ contract ImplementationModuleSingleEndpointTest is ImplementationFixture { // Verify storage is not modified by upgrades in `Dispatcher` context. _verifyGetStateSlot(message_); + + // Attempt to upgrade single-endpoint module that was marked as deprecated, this should fail. + + moduleAddresses = new address[](1); + moduleAddresses[0] = address(singleModuleV4); + + vm.expectRevert( + abi.encodeWithSelector(IReflexInstaller.ModuleNotUpgradeable.selector, singleModuleV4.moduleId()) + ); + installerEndpoint.upgradeModules(moduleAddresses); + + // Verify single-endpoint module was not upgraded. + + _verifyModuleConfiguration( + singleModuleEndpoint, + _MODULE_SINGLE_ID, + _MODULE_SINGLE_TYPE, + _MODULE_SINGLE_VERSION_V3, + _MODULE_SINGLE_UPGRADEABLE_V3 + ); + + // Verify storage is not modified by upgrades in `Dispatcher` context. + + _verifyGetStateSlot(message_); + } + + function testFuzzUpgradeSingleModuleToMaliciousStorageModule( + bytes32 message_, + uint8 number_ + ) external brutalizeMemory { + vm.assume(uint8(uint256(message_)) != number_); + + // Verify storage sets in `Dispatcher` context. + + _verifySetStateSlot(message_); + + _verifyModuleConfiguration( + singleModuleEndpoint, + _MODULE_SINGLE_ID, + _MODULE_SINGLE_TYPE, + _MODULE_SINGLE_VERSION_V1, + _MODULE_SINGLE_UPGRADEABLE_V1 + ); + + // Upgrade single-endpoint module to malicious storage module. + + address[] memory moduleAddresses = new address[](1); + moduleAddresses[0] = address(singleModuleMaliciousStorageV3); + installerEndpoint.upgradeModules(moduleAddresses); + + _verifyModuleConfiguration( + singleModuleEndpoint, + _MODULE_SINGLE_ID, + _MODULE_SINGLE_TYPE, + _MODULE_SINGLE_VERSION_V3, + _MODULE_SINGLE_UPGRADEABLE_V3 + ); + + // Overwrite storage in the `Dispatcher` context from the malicious module. + + MockImplementationMaliciousStorageModule(address(singleModuleEndpoint)).setNumber(number_); + + // Verify storage has been modified by malicious upgrade in `Dispatcher` context. + + assertEq(MockImplementationMaliciousStorageModule(address(singleModuleEndpoint)).getNumber(), number_); + + // Verify that the storage in the `Dispatcher` context has been overwritten, this is disastrous. + + assertEq(uint8(uint256(dispatcher.getImplementationState0())), number_); + assertFalse(dispatcher.getImplementationState0() == message_); + + // Overwrite storage in the `Dispatcher` context. + + dispatcher.setImplementationState0(message_); + + // Verify that the storage in the `Dispatcher` context has been overwritten. + + assertEq(dispatcher.getImplementationState0(), message_); + assertFalse(uint8(uint256(dispatcher.getImplementationState0())) == number_); } function testUnitEndpointSentinelFallback() external { diff --git a/test/ImplementationState.t.sol b/test/ImplementationState.t.sol index b57e8ee..7a4e0d4 100644 --- a/test/ImplementationState.t.sol +++ b/test/ImplementationState.t.sol @@ -29,9 +29,9 @@ contract ImplementationStateTest is TestHarness { // Tests // ===== - function testFuzzVerifyStorageSlot(bytes32 message_) external brutalizeMemory { - state.setStorageSlot(message_); - state.verifyStorageSlot(message_); + function testFuzzVerifyStorageSlot0(bytes32 message_) external brutalizeMemory { + state.setStorageSlot0(message_); + state.verifyStorageSlot0(message_); } function testFuzzVerifyStorageSlots( diff --git a/test/ReflexBatch.t.sol b/test/ReflexBatch.t.sol index cb2550c..bf811bb 100644 --- a/test/ReflexBatch.t.sol +++ b/test/ReflexBatch.t.sol @@ -15,7 +15,6 @@ import {ReflexFixture} from "./fixtures/ReflexFixture.sol"; // Mocks import {ImplementationERC20} from "./mocks/abstracts/ImplementationERC20.sol"; import {MockImplementationERC20} from "./mocks/MockImplementationERC20.sol"; -import {MockERC20} from "./mocks/MockERC20.sol"; import {MockImplementationERC20Hub} from "./mocks/MockImplementationERC20Hub.sol"; import {MockImplementationModule} from "./mocks/MockImplementationModule.sol"; import {MockReflexBatch} from "./mocks/MockReflexBatch.sol"; @@ -66,7 +65,7 @@ contract ReflexBatchTest is ReflexFixture { // Storage // ======= - MockERC20 public token; + ExternalTarget public externalTarget; MockReflexBatch public batch; MockReflexBatch public batchEndpoint; @@ -86,7 +85,7 @@ contract ReflexBatchTest is ReflexFixture { function setUp() public virtual override { super.setUp(); - token = new MockERC20(_MODULE_MULTI_NAME_A, _MODULE_MULTI_SYMBOL_A, _MODULE_MULTI_DECIMALS_A); + externalTarget = new ExternalTarget(); batch = new MockReflexBatch( IReflexModule.ModuleSettings({ @@ -146,10 +145,7 @@ contract ReflexBatchTest is ReflexFixture { ); } - function testFuzzStaticCall( - address target_, - uint256 amount_ - ) external withHooksExpected(1) withExternalToken(target_, amount_) { + function testFuzzStaticCall(uint256 amount_) external withHooksExpected(1) withExternalTarget(amount_) { IReflexBatch.BatchAction[] memory actions = new IReflexBatch.BatchAction[](1); actions[0] = IReflexBatch.BatchAction({ @@ -157,7 +153,7 @@ contract ReflexBatchTest is ReflexFixture { endpointAddress: address(batchEndpoint), callData: abi.encodeCall( batchEndpoint.performStaticCall, - (address(token), abi.encodeCall(IERC20.balanceOf, (target_))) + (address(externalTarget), abi.encodeCall(ExternalTarget.getNumber, ())) ) }); @@ -178,11 +174,11 @@ contract ReflexBatchTest is ReflexFixture { endpointAddress: address(batchEndpoint), callData: abi.encodeCall( batchEndpoint.performStaticCall, - (address(token), abi.encodeCall(MockERC20.getRevert, ())) + (address(externalTarget), abi.encodeCall(ExternalTarget.getRevertStaticCall, ())) ) }); - vm.expectRevert(MockERC20.KnownViewError.selector); + vm.expectRevert(ExternalTarget.KnownViewError.selector); batchEndpoint.performBatchCall(actions); } @@ -190,7 +186,7 @@ contract ReflexBatchTest is ReflexFixture { address target_, uint256 amount_, bytes32 message_ - ) external withHooksExpected(1) withExternalToken(target_, amount_) { + ) external withHooksExpected(1) withExternalTarget(amount_) { IReflexBatch.BatchAction[] memory actions = new IReflexBatch.BatchAction[](7); actions[0] = IReflexBatch.BatchAction({ @@ -234,7 +230,7 @@ contract ReflexBatchTest is ReflexFixture { endpointAddress: address(batchEndpoint), callData: abi.encodeCall( batchEndpoint.performStaticCall, - (address(token), abi.encodeCall(IERC20.balanceOf, (target_))) + (address(externalTarget), abi.encodeCall(ExternalTarget.getNumber, ())) ) }); @@ -264,7 +260,7 @@ contract ReflexBatchTest is ReflexFixture { address target_, uint256 amount_, bytes32 message_ - ) external withHooksExpected(1) withExternalToken(target_, amount_) { + ) external withHooksExpected(1) withExternalTarget(amount_) { IReflexBatch.BatchAction[] memory actions = new IReflexBatch.BatchAction[](7); actions[0] = IReflexBatch.BatchAction({ @@ -308,7 +304,7 @@ contract ReflexBatchTest is ReflexFixture { endpointAddress: address(batchEndpoint), callData: abi.encodeCall( batchEndpoint.performStaticCall, - (address(token), abi.encodeCall(IERC20.balanceOf, (target_))) + (address(externalTarget), abi.encodeCall(ExternalTarget.getNumber, ())) ) }); @@ -368,7 +364,7 @@ contract ReflexBatchTest is ReflexFixture { address target_, uint256 amount_, bytes32 message_ - ) external withHooksExpected(1) withExternalToken(target_, amount_) { + ) external withHooksExpected(1) withExternalTarget(amount_) { IReflexBatch.BatchAction[] memory actions = new IReflexBatch.BatchAction[](7); actions[0] = IReflexBatch.BatchAction({ @@ -412,7 +408,7 @@ contract ReflexBatchTest is ReflexFixture { endpointAddress: address(batchEndpoint), callData: abi.encodeCall( batchEndpoint.performStaticCall, - (address(token), abi.encodeCall(IERC20.balanceOf, (target_))) + (address(externalTarget), abi.encodeCall(ExternalTarget.getNumber, ())) ) }); @@ -549,16 +545,48 @@ contract ReflexBatchTest is ReflexFixture { assertEq(batchEndpoint.afterBatchCallCounter(), batchCallCounter_); } - modifier withExternalToken(address target_, uint256 amount_) { - token.mint(target_, amount_); + modifier withExternalTarget(uint256 number_) { + externalTarget.setNumber(number_); + + _; + + assertEq(externalTarget.getNumber(), number_); + } +} - (bool success, bytes memory result) = address(token).staticcall(abi.encodeCall(IERC20.balanceOf, (target_))); +// ========= +// Utilities +// ========= - assertTrue(success); - assertEq(abi.decode(result, (uint256)), amount_); +/** + * @title External Target + */ +contract ExternalTarget { + // ====== + // Errors + // ====== - _; + error KnownViewError(); + + // ======= + // Storage + // ======= + + uint256 internal _number; + + // ========== + // Test stubs + // ========== + + function getRevertStaticCall() external pure { + revert KnownViewError(); + } + + function getNumber() external view returns (uint256) { + return _number; + } - assertEq(token.balanceOf(target_), amount_); + function setNumber(uint256 number_) external { + _number = number_; } } diff --git a/test/mocks/MockERC20.sol b/test/mocks/MockERC20.sol deleted file mode 100644 index 810e737..0000000 --- a/test/mocks/MockERC20.sol +++ /dev/null @@ -1,39 +0,0 @@ -// // SPDX-License-Identifier: GPL-3.0-or-later -pragma solidity ^0.8.13; - -// Mocks -import {ExternalERC20} from "./abstracts/ExternalERC20.sol"; - -/** - * @title Mock ERC20 - */ -contract MockERC20 is ExternalERC20 { - // ====== - // Errors - // ====== - - error KnownViewError(); - - // =========== - // Constructor - // =========== - - /** - * @param name_ Token name. - * @param symbol_ Token symbol. - * @param decimals_ Token decimals. - */ - constructor(string memory name_, string memory symbol_, uint8 decimals_) ExternalERC20(name_, symbol_, decimals_) {} - - // ========== - // Test stubs - // ========== - - function getRevert() external pure { - revert KnownViewError(); - } - - function mint(address to, uint256 value) external { - _mint(to, value); - } -} diff --git a/test/mocks/MockImplementationMaliciousModule.sol b/test/mocks/MockImplementationMaliciousStorageModule.sol similarity index 85% rename from test/mocks/MockImplementationMaliciousModule.sol rename to test/mocks/MockImplementationMaliciousStorageModule.sol index 4afc21f..da600e7 100644 --- a/test/mocks/MockImplementationMaliciousModule.sol +++ b/test/mocks/MockImplementationMaliciousStorageModule.sol @@ -5,9 +5,9 @@ pragma solidity ^0.8.13; import {MockReflexModule} from "./MockReflexModule.sol"; /** - * @title Mock Implementation Malicious Module + * @title Mock Implementation Malicious Storage Module */ -contract MockImplementationMaliciousModule is MockReflexModule { +contract MockImplementationMaliciousStorageModule is MockReflexModule { // ======= // Storage // ======= diff --git a/test/mocks/MockImplementationState.sol b/test/mocks/MockImplementationState.sol index f0adcff..5fe3ef5 100644 --- a/test/mocks/MockImplementationState.sol +++ b/test/mocks/MockImplementationState.sol @@ -19,10 +19,20 @@ contract MockImplementationState is ImplementationState, StdAssertions, CommonBa // Test stubs // ========== - function setStorageSlot(bytes32 message_) public { + function setStorageSlot0(bytes32 message_) public { setImplementationState0(message_); } + function verifyStorageSlot0(bytes32 message_) public { + /** + * | Name | Type | Slot | Offset | Bytes | + * |-------------------------|------------------------------------------------------|------|--------|-------| + * | _implementationState0 | bytes32 | 50 | 0 | 32 | + */ + assertEq(stdstore.target(address(this)).sig("getImplementationState0()").find(), 50); + assertEq(stdstore.target(address(this)).sig("getImplementationState0()").read_bytes32(), message_); + } + function setStorageSlots( bytes32 message_, uint256 number_, @@ -42,16 +52,6 @@ contract MockImplementationState is ImplementationState, StdAssertions, CommonBa setToken(tokenB_, "Token B", "TKNB", 18); } - function verifyStorageSlot(bytes32 message_) public { - /** - * | Name | Type | Slot | Offset | Bytes | - * |-------------------------|------------------------------------------------------|------|--------|-------| - * | _implementationState0 | bytes32 | 50 | 0 | 32 | - */ - assertEq(stdstore.target(address(this)).sig("getImplementationState0()").find(), 50); - assertEq(stdstore.target(address(this)).sig("getImplementationState0()").read_bytes32(), message_); - } - function verifyStorageSlots( bytes32 message_, uint256 number_, diff --git a/test/mocks/MockReflexBase.sol b/test/mocks/MockReflexBase.sol index a32ef65..79bca93 100644 --- a/test/mocks/MockReflexBase.sol +++ b/test/mocks/MockReflexBase.sol @@ -149,6 +149,9 @@ contract MockReflexBase is ReflexBase { // Utilities // ========= +/** + * @title Reentrancy Attack + */ contract ReentrancyAttack { // ====== // Errors diff --git a/test/mocks/abstracts/ExternalERC20.sol b/test/mocks/abstracts/ExternalERC20.sol deleted file mode 100644 index 680559f..0000000 --- a/test/mocks/abstracts/ExternalERC20.sol +++ /dev/null @@ -1,198 +0,0 @@ -// // SPDX-License-Identifier: GPL-3.0-or-later -pragma solidity ^0.8.13; - -// Vendor -import {IERC20} from "forge-std/interfaces/IERC20.sol"; - -/** - * @title External ERC20 - * - * @author `ERC20` has been modified from: Solmate - * (https://github.com/transmissions11/solmate/blob/v7/src/tokens/ERC20.sol) (MIT) - */ -abstract contract ExternalERC20 is IERC20 { - // ========== - // Immutables - // ========== - - uint8 public immutable decimals; - - uint256 internal immutable _initialChainId; - - bytes32 internal immutable _initialDomainSeparator; - - // ======= - // Storage - // ======= - - string public name; - - string public symbol; - - uint256 public totalSupply; - - mapping(address => uint256) public balanceOf; - - mapping(address => mapping(address => uint256)) public allowance; - - mapping(address => uint256) public nonces; - - // =========== - // Constructor - // =========== - - /** - * @param name_ Token name. - * @param symbol_ Token symbol. - * @param decimals_ Token decimals. - */ - constructor(string memory name_, string memory symbol_, uint8 decimals_) { - name = name_; - symbol = symbol_; - decimals = decimals_; - - _initialChainId = block.chainid; - _initialDomainSeparator = _computeDomainSeparator(); - } - - // ============ - // View methods - // ============ - - // solhint-disable-next-line func-name-mixedcase - function DOMAIN_SEPARATOR() public view virtual returns (bytes32) { - return block.chainid == _initialChainId ? _initialDomainSeparator : _computeDomainSeparator(); - } - - // ============== - // Public methods - // ============== - - function approve(address spender, uint256 amount) public virtual returns (bool) { - allowance[msg.sender][spender] = amount; - - emit Approval(msg.sender, spender, amount); - - return true; - } - - function transfer(address to, uint256 amount) public virtual returns (bool) { - balanceOf[msg.sender] -= amount; - - // Cannot overflow because the sum of all user - // balances can't exceed the max uint256 value. - unchecked { - balanceOf[to] += amount; - } - - emit Transfer(msg.sender, to, amount); - - return true; - } - - function transferFrom(address from, address to, uint256 amount) public virtual returns (bool) { - uint256 allowed = allowance[from][msg.sender]; // Saves gas for limited approvals. - - if (allowed != type(uint256).max) allowance[from][msg.sender] = allowed - amount; - - balanceOf[from] -= amount; - - // Cannot overflow because the sum of all user - // balances can't exceed the max uint256 value. - unchecked { - balanceOf[to] += amount; - } - - emit Transfer(from, to, amount); - - return true; - } - - function permit( - address owner, - address spender, - uint256 value, - uint256 deadline, - uint8 v, - bytes32 r, - bytes32 s - ) public virtual { - // solhint-disable-next-line not-rely-on-time - require(deadline >= block.timestamp, "PERMIT_DEADLINE_EXPIRED"); - - // Unchecked because the only math done is incrementing - // the owner's nonce which cannot realistically overflow. - unchecked { - address recoveredAddress = ecrecover( - keccak256( - abi.encodePacked( - "\x19\x01", - DOMAIN_SEPARATOR(), - keccak256( - abi.encode( - keccak256( - "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" - ), - owner, - spender, - value, - nonces[owner]++, - deadline - ) - ) - ) - ), - v, - r, - s - ); - - require(recoveredAddress != address(0) && recoveredAddress == owner, "INVALID_SIGNER"); - - allowance[recoveredAddress][spender] = value; - } - - emit Approval(owner, spender, value); - } - - // ================ - // Internal methods - // ================ - - function _computeDomainSeparator() internal view virtual returns (bytes32) { - return - keccak256( - abi.encode( - keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), - keccak256(bytes(name)), - keccak256("1"), - block.chainid, - address(this) - ) - ); - } - - function _mint(address to, uint256 amount) internal virtual { - totalSupply += amount; - - // Cannot overflow because the sum of all user - // balances can't exceed the max uint256 value. - unchecked { - balanceOf[to] += amount; - } - - emit Transfer(address(0), to, amount); - } - - function _burn(address from, uint256 amount) internal virtual { - balanceOf[from] -= amount; - - // Cannot underflow because a user's balance - // will never be larger than the total supply. - unchecked { - totalSupply -= amount; - } - - emit Transfer(from, address(0), amount); - } -}