From 4df3093349a7e56db431590a349d5bd02e57e675 Mon Sep 17 00:00:00 2001 From: Elliot Date: Thu, 9 Feb 2023 18:06:37 -0800 Subject: [PATCH 1/4] remove reentrancy checks when lock is set to address 0 --- src/refs/CoreRefV2.sol | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/refs/CoreRefV2.sol b/src/refs/CoreRefV2.sol index 1af26f79..30706692 100644 --- a/src/refs/CoreRefV2.sol +++ b/src/refs/CoreRefV2.sol @@ -33,17 +33,29 @@ abstract contract CoreRefV2 is ICoreRefV2, Pausable { /// 3. call core and unlock the lock back to starting level modifier globalLock(uint8 level) { IGlobalReentrancyLock lock = globalReentrancyLock(); - lock.lock(level); - _; - lock.unlock(level - 1); + + if (address(lock) != address(0)) { + lock.lock(level); + _; + lock.unlock(level - 1); + } else { + _; /// if lock is not set, allow function execution without global reentrancy locks + } } /// @notice modifier to restrict function acces to a certain lock level modifier isGlobalReentrancyLocked(uint8 level) { IGlobalReentrancyLock lock = globalReentrancyLock(); - require(lock.lockLevel() == level, "CoreRef: System not at lock level"); - _; + if (address(lock) != address(0)) { + require( + lock.lockLevel() == level, + "CoreRef: System not at lock level" + ); + _; + } else { + _; /// if lock is not set, allow function execution without global lock level + } } /// @notice callable only by the Volt Minter From 9f7cd7730c86d1c3dd8ef9be6dc8d6afa25d94ed Mon Sep 17 00:00:00 2001 From: Elliot Date: Thu, 9 Feb 2023 18:07:16 -0800 Subject: [PATCH 2/4] add lock tests --- test/mock/MockCoreRefV2.sol | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/mock/MockCoreRefV2.sol b/test/mock/MockCoreRefV2.sol index f3eae9ed..7df17335 100644 --- a/test/mock/MockCoreRefV2.sol +++ b/test/mock/MockCoreRefV2.sol @@ -17,6 +17,17 @@ contract MockCoreRefV2 is CoreRefV2 { function testSystemState() public onlyVoltRole(VoltRoles.LOCKER) {} + function testSystemLocksToLevel1() public globalLock(1) {} + + function testSystemLocksToLevel2() public globalLock(2) {} + + /// invalid lock level, doesn't matter because the lock is disabled in test + function testSystemLocksToLevel3() public globalLock(3) {} + + function testSystemLockLevel1() public isGlobalReentrancyLocked(1) {} + + function testSystemLockLevel2() public isGlobalReentrancyLocked(2) {} + function testStateGovernorMinter() public hasAnyOfThreeRoles( From 120fa5e0e7d9810b2e9ffe42774083ccc769bc95 Mon Sep 17 00:00:00 2001 From: Elliot Date: Thu, 9 Feb 2023 18:07:52 -0800 Subject: [PATCH 3/4] add additional reentrancy tests when lock is set to 0 --- test/unit/refs/CoreRefV2.t.sol | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/test/unit/refs/CoreRefV2.t.sol b/test/unit/refs/CoreRefV2.t.sol index aa4f778c..ea8f3c60 100644 --- a/test/unit/refs/CoreRefV2.t.sol +++ b/test/unit/refs/CoreRefV2.t.sol @@ -6,10 +6,11 @@ import {Test} from "@forge-std/Test.sol"; import {ICoreV2} from "@voltprotocol/core/ICoreV2.sol"; import {VoltRoles} from "@voltprotocol/core/VoltRoles.sol"; import {MockERC20} from "@test/mock/MockERC20.sol"; +import {getCoreV2} from "@test/unit/utils/Fixtures.sol"; import {IPCVOracle} from "@voltprotocol/oracle/IPCVOracle.sol"; import {MockCoreRefV2} from "@test/mock/MockCoreRefV2.sol"; import {TestAddresses as addresses} from "@test/unit/utils/TestAddresses.sol"; -import {getCoreV2} from "@test/unit/utils/Fixtures.sol"; +import {IGlobalReentrancyLock} from "@voltprotocol/core/IGlobalReentrancyLock.sol"; contract UnitTestCoreRefV2 is Test { ICoreV2 private core; @@ -113,6 +114,36 @@ contract UnitTestCoreRefV2 is Test { coreRef.testSystemState(); } + function _disableLock() private { + vm.prank(addresses.governorAddress); + core.setGlobalReentrancyLock(IGlobalReentrancyLock(address(0))); + } + + function testLockLevel1LockDisabled() public { + _disableLock(); + coreRef.testSystemLocksToLevel1(); + } + + function testLockLevel2LockDisabled() public { + _disableLock(); + coreRef.testSystemLocksToLevel2(); + } + + function testLockLevel3LockDisabled() public { + _disableLock(); + coreRef.testSystemLocksToLevel3(); + } + + function testSystemLockLevel1LockDisabled() public { + _disableLock(); + coreRef.testSystemLockLevel1(); + } + + function testSystemLockLevel2LockDisabled() public { + _disableLock(); + coreRef.testSystemLockLevel2(); + } + function testGuardianAsGuardian() public { vm.prank(addresses.guardianAddress); coreRef.testGuardian(); From 527f1e32ad547d88afdc66f9bc13abfc8c32646e Mon Sep 17 00:00:00 2001 From: Elliot Date: Thu, 9 Feb 2023 18:08:08 -0800 Subject: [PATCH 4/4] update comments --- test/unit/core/GlobalReentrancyLock.t.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/core/GlobalReentrancyLock.t.sol b/test/unit/core/GlobalReentrancyLock.t.sol index 06bc2699..e04de95e 100644 --- a/test/unit/core/GlobalReentrancyLock.t.sol +++ b/test/unit/core/GlobalReentrancyLock.t.sol @@ -49,8 +49,8 @@ contract UnitTestGlobalReentrancyLock is Test { assertTrue(core.isGovernor(address(core))); /// core contract is governor assertTrue(core.isGovernor(addresses.governorAddress)); /// msg.sender of contract is governor - assertTrue(lock.isUnlocked()); /// core starts out unlocked - assertTrue(!lock.isLocked()); /// core starts out not locked + assertTrue(lock.isUnlocked()); /// lock starts out unlocked + assertTrue(!lock.isLocked()); /// lock starts out not locked assertEq(lock.lastSender(), address(0)); assertEq(lock.lastBlockEntered(), 0); @@ -219,7 +219,7 @@ contract UnitTestGlobalReentrancyLock is Test { assertTrue(!lock.isLocked()); assertEq(lock.lockLevel(), 0); - assertTrue(lock.isUnlocked()); /// core is fully unlocked + assertTrue(lock.isUnlocked()); /// lock is fully unlocked assertEq(toPrank, lock.lastSender()); vm.roll(block.number + 1);