Merge pull request #5 from GalloDaSballo/feat-tinfoil

Fix: Try Catch that actually doesn't revert
liquity · Oct 10, 2024 · 86f7906 · 86f7906
2 parents bcf897e + 08d897c
commit 86f7906
Show file tree

Hide file tree

Showing 4 changed files with 393 additions and 6 deletions.
diff --git a/src/Governance.sol b/src/Governance.sol
@@ -15,11 +15,14 @@ import {UserProxyFactory} from "./UserProxyFactory.sol";
 import {add, max} from "./utils/Math.sol";
 import {Multicall} from "./utils/Multicall.sol";
 import {WAD, PermitParams} from "./utils/Types.sol";
+import {safeCallWithMinGas} from "./utils/SafeCallMinGas.sol";
 
 /// @title Governance: Modular Initiative based Governance
 contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance {
     using SafeERC20 for IERC20;
 
+    uint256 constant MIN_GAS_TO_HOOK = 350_000; /// Replace this to ensure hooks have sufficient gas
+
     /// @inheritdoc IGovernance
     ILQTYStaking public immutable stakingV1;
     /// @inheritdoc IGovernance
@@ -323,7 +326,9 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
 
         emit RegisterInitiative(_initiative, msg.sender, currentEpoch);
 
-        try IInitiative(_initiative).onRegisterInitiative(currentEpoch) {} catch {}
+        // try IInitiative(_initiative).onRegisterInitiative(currentEpoch) {} catch {}
+        // Replaces try / catch | Enforces sufficient gas is passed
+        safeCallWithMinGas(_initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onRegisterInitiative, (currentEpoch)));
     }
 
     /// @inheritdoc IGovernance
@@ -369,7 +374,9 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
 
         emit UnregisterInitiative(_initiative, currentEpoch);
 
-        try IInitiative(_initiative).onUnregisterInitiative(currentEpoch) {} catch {}
+        // try IInitiative(_initiative).onUnregisterInitiative(currentEpoch) {} catch {}
+        // Replaces try / catch | Enforces sufficient gas is passed
+        safeCallWithMinGas(_initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onUnregisterInitiative, (currentEpoch)));
     }
 
     /// @inheritdoc IGovernance
@@ -477,9 +484,11 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
 
             emit AllocateLQTY(msg.sender, initiative, deltaLQTYVotes, deltaLQTYVetos, currentEpoch);
 
-            try IInitiative(initiative).onAfterAllocateLQTY(
-                currentEpoch, msg.sender, allocation.voteLQTY, allocation.vetoLQTY
-            ) {} catch {}
+            // try IInitiative(initiative).onAfterAllocateLQTY(
+            //     currentEpoch, msg.sender, allocation.voteLQTY, allocation.vetoLQTY
+            // ) {} catch {}
+            // Replaces try / catch | Enforces sufficient gas is passed
+            safeCallWithMinGas(initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onAfterAllocateLQTY, (currentEpoch, msg.sender, allocation.voteLQTY, allocation.vetoLQTY)));
         }
 
         require(
@@ -509,7 +518,9 @@ contract Governance is Multicall, UserProxyFactory, ReentrancyGuard, IGovernance
 
         emit ClaimForInitiative(_initiative, claim, votesSnapshot_.forEpoch);
 
-        try IInitiative(_initiative).onClaimForInitiative(votesSnapshot_.forEpoch, claim) {} catch {}
+        // try IInitiative(_initiative).onClaimForInitiative(votesSnapshot_.forEpoch, claim) {} catch {}
+        // Replaces try / catch | Enforces sufficient gas is passed
+        safeCallWithMinGas(_initiative, MIN_GAS_TO_HOOK, 0, abi.encodeCall(IInitiative.onClaimForInitiative, (votesSnapshot_.forEpoch, claim)));
 
         return claim;
     }

diff --git a/src/utils/SafeCallMinGas.sol b/src/utils/SafeCallMinGas.sol
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.24;
+
+/// @notice Given the gas requirement, ensures that the current context has sufficient gas to perform a call + a fixed buffer
+/// @dev Credits: https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/src/libraries/SafeCall.sol#L100-L107
+function hasMinGas(uint256 _minGas, uint256 _reservedGas) view returns (bool) {
+    bool _hasMinGas;
+    assembly {
+        // Equation: gas × 63 ≥ minGas × 64 + 63(40_000 + reservedGas)
+        _hasMinGas := iszero(lt(mul(gas(), 63), add(mul(_minGas, 64), mul(add(40000, _reservedGas), 63))))
+    }
+    return _hasMinGas;
+}
+
+/// @dev Performs a call ignoring the recipient existing or not, passing the exact gas value, ignoring any return value
+function safeCallWithMinGas(
+    address _target,
+    uint256 _gas,
+    uint256 _value,
+    bytes memory _calldata
+) returns (bool success) {
+    /// @audit This is not necessary
+    ///     But this is basically a worst case estimate of mem exp cost + operations before the call
+    require(hasMinGas(_gas, 1_000), "Must have minGas"); 
+
+    // dispatch message to recipient
+    // by assembly calling "handle" function
+    // we call via assembly to avoid memcopying a very large returndata
+    // returned by a malicious contract
+    assembly {
+        success := call(
+            _gas, // gas
+            _target, // recipient
+            _value, // ether value
+            add(_calldata, 0x20), // inloc
+            mload(_calldata), // inlen
+            0, // outloc
+            0 // outlen
+        )
+
+        // Ignore all return values
+    }
+    return (success);
+}
diff --git a/test/GovernanceAttacks.t.sol b/test/GovernanceAttacks.t.sol
@@ -0,0 +1,255 @@
+// SPDX-License-Identifier: UNLICENSED
+pragma solidity ^0.8.24;
+
+import {Test} from "forge-std/Test.sol";
+import {VmSafe} from "forge-std/Vm.sol";
+import {console} from "forge-std/console.sol";
+
+import {IERC20} from "openzeppelin-contracts/contracts/interfaces/IERC20.sol";
+
+import {IGovernance} from "../src/interfaces/IGovernance.sol";
+import {ILQTY} from "../src/interfaces/ILQTY.sol";
+
+import {BribeInitiative} from "../src/BribeInitiative.sol";
+import {Governance} from "../src/Governance.sol";
+import {UserProxy} from "../src/UserProxy.sol";
+
+import {PermitParams} from "../src/utils/Types.sol";
+
+import {MaliciousInitiative} from "./mocks/MaliciousInitiative.sol";
+
+
+contract GovernanceTest is Test {
+    IERC20 private constant lqty = IERC20(address(0x6DEA81C8171D0bA574754EF6F8b412F2Ed88c54D));
+    IERC20 private constant lusd = IERC20(address(0x5f98805A4E8be255a32880FDeC7F6728C6568bA0));
+    address private constant stakingV1 = address(0x4f9Fbb3f1E99B56e0Fe2892e623Ed36A76Fc605d);
+    address private constant user = address(0xF977814e90dA44bFA03b6295A0616a897441aceC);
+    address private constant user2 = address(0x10C9cff3c4Faa8A60cB8506a7A99411E6A199038);
+    address private constant lusdHolder = address(0xcA7f01403C4989d2b1A9335A2F09dD973709957c);
+
+    uint128 private constant REGISTRATION_FEE = 1e18;
+    uint128 private constant REGISTRATION_THRESHOLD_FACTOR = 0.01e18;
+    uint128 private constant UNREGISTRATION_THRESHOLD_FACTOR = 4e18;
+    uint16 private constant REGISTRATION_WARM_UP_PERIOD = 4;
+    uint16 private constant UNREGISTRATION_AFTER_EPOCHS = 4;
+    uint128 private constant VOTING_THRESHOLD_FACTOR = 0.04e18;
+    uint88 private constant MIN_CLAIM = 500e18;
+    uint88 private constant MIN_ACCRUAL = 1000e18;
+    uint32 private constant EPOCH_DURATION = 604800;
+    uint32 private constant EPOCH_VOTING_CUTOFF = 518400;
+
+    Governance private governance;
+    address[] private initialInitiatives;
+
+    MaliciousInitiative private maliciousInitiative1;
+    MaliciousInitiative private maliciousInitiative2;
+    MaliciousInitiative private eoaInitiative;
+
+    function setUp() public {
+        vm.createSelectFork(vm.rpcUrl("mainnet"), 20430000);
+
+        maliciousInitiative1 = new MaliciousInitiative();
+        maliciousInitiative2 = new MaliciousInitiative();
+        eoaInitiative = MaliciousInitiative(address(0x123123123123));
+
+        initialInitiatives.push(address(maliciousInitiative1));
+
+        governance = new Governance(
+            address(lqty),
+            address(lusd),
+            stakingV1,
+            address(lusd),
+            IGovernance.Configuration({
+                registrationFee: REGISTRATION_FEE,
+                registrationThresholdFactor: REGISTRATION_THRESHOLD_FACTOR,
+                unregistrationThresholdFactor: UNREGISTRATION_THRESHOLD_FACTOR,
+                registrationWarmUpPeriod: REGISTRATION_WARM_UP_PERIOD,
+                unregistrationAfterEpochs: UNREGISTRATION_AFTER_EPOCHS,
+                votingThresholdFactor: VOTING_THRESHOLD_FACTOR,
+                minClaim: MIN_CLAIM,
+                minAccrual: MIN_ACCRUAL,
+                epochStart: uint32(block.timestamp),
+                epochDuration: EPOCH_DURATION,
+                epochVotingCutoff: EPOCH_VOTING_CUTOFF
+            }),
+            initialInitiatives
+        );
+
+    }
+
+    // forge test --match-test test_deposit_attack -vv
+    // All calls should never revert due to malicious initiative
+    function test_all_revert_attacks_hardcoded() public {
+        uint256 zeroSnapshot = vm.snapshot();
+        uint256 timeIncrease = 86400 * 30;
+        vm.warp(block.timestamp + timeIncrease);
+
+        vm.startPrank(user);
+
+        // should not revert if the user doesn't have a UserProxy deployed yet
+        address userProxy = governance.deriveUserProxyAddress(user);
+        lqty.approve(address(userProxy), 1e18);
+
+        // deploy and deposit 1 LQTY
+        governance.depositLQTY(1e18);
+        assertEq(UserProxy(payable(userProxy)).staked(), 1e18);
+        (uint88 allocatedLQTY, uint32 averageStakingTimestamp) = governance.userStates(user);
+        assertEq(allocatedLQTY, 0);
+        // first deposit should have an averageStakingTimestamp if block.timestamp
+        assertEq(averageStakingTimestamp, block.timestamp);
+        vm.warp(block.timestamp + timeIncrease);
+        vm.stopPrank();
+
+
+        vm.startPrank(lusdHolder);
+        lusd.transfer(address(governance), 10000e18);
+        vm.stopPrank();
+
+        address maliciousWhale = address(0xb4d);
+        deal(address(lusd), maliciousWhale, 2000e18);
+        vm.startPrank(maliciousWhale);
+        lusd.approve(address(governance), type(uint256).max);
+
+        /// === REGISTRATION REVERTS === ///
+        uint256 registerNapshot = vm.snapshot();
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.THROW);
+        governance.registerInitiative(address(maliciousInitiative2));
+        vm.revertTo(registerNapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.OOG);
+        governance.registerInitiative(address(maliciousInitiative2));
+        vm.revertTo(registerNapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.RETURN_BOMB);
+        governance.registerInitiative(address(maliciousInitiative2));
+        vm.revertTo(registerNapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.REVERT_BOMB);
+        governance.registerInitiative(address(maliciousInitiative2));
+        vm.revertTo(registerNapshot);
+
+        // Reset and continue
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.REGISTER, MaliciousInitiative.RevertType.NONE);
+        governance.registerInitiative(address(maliciousInitiative2));
+
+        // Register EOA
+        governance.registerInitiative(address(eoaInitiative));
+
+
+        vm.stopPrank();
+
+        vm.warp(block.timestamp + governance.EPOCH_DURATION());
+
+        vm.startPrank(user);
+        address[] memory initiatives = new address[](2);
+        initiatives[0] = address(maliciousInitiative2);
+        initiatives[1] = address(eoaInitiative);
+        int176[] memory deltaVoteLQTY = new int176[](2);
+        deltaVoteLQTY[0] = 5e17;
+        deltaVoteLQTY[1] = 5e17;
+        int176[] memory deltaVetoLQTY = new int176[](2);
+
+        /// === Allocate LQTY REVERTS === ///
+        uint256 allocateSnapshot = vm.snapshot();
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.ALLOCATE, MaliciousInitiative.RevertType.THROW);
+        governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);
+        vm.revertTo(allocateSnapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.ALLOCATE, MaliciousInitiative.RevertType.OOG);
+        governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);
+        vm.revertTo(allocateSnapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.ALLOCATE, MaliciousInitiative.RevertType.RETURN_BOMB);
+        governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);
+        vm.revertTo(allocateSnapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.ALLOCATE, MaliciousInitiative.RevertType.REVERT_BOMB);
+        governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);
+        vm.revertTo(allocateSnapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.ALLOCATE, MaliciousInitiative.RevertType.NONE);
+        governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);
+
+
+
+        vm.warp(block.timestamp + governance.EPOCH_DURATION() + 1);
+
+        /// === Claim for initiative REVERTS === ///
+        uint256 claimShapsnot = vm.snapshot();
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.THROW);
+        governance.claimForInitiative(address(maliciousInitiative2));
+        vm.revertTo(claimShapsnot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.OOG);
+        governance.claimForInitiative(address(maliciousInitiative2));
+        vm.revertTo(claimShapsnot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.RETURN_BOMB);
+        governance.claimForInitiative(address(maliciousInitiative2));
+        vm.revertTo(claimShapsnot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.REVERT_BOMB);
+        governance.claimForInitiative(address(maliciousInitiative2));
+        vm.revertTo(claimShapsnot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.CLAIM, MaliciousInitiative.RevertType.NONE);
+        uint256 claimed = governance.claimForInitiative(address(maliciousInitiative2));
+
+        governance.claimForInitiative(address(eoaInitiative));
+
+
+        /// === Unregister Reverts === ///
+
+        vm.startPrank(user);
+        initiatives = new address[](3);
+        initiatives[0] = address(maliciousInitiative2);
+        initiatives[1] = address(eoaInitiative);
+        initiatives[2] = address(maliciousInitiative1);
+        deltaVoteLQTY = new int176[](3);
+        deltaVoteLQTY[0] = -5e17;
+        deltaVoteLQTY[1] = -5e17;
+        deltaVoteLQTY[2] = 5e17;
+        deltaVetoLQTY = new int176[](3);
+        governance.allocateLQTY(initiatives, deltaVoteLQTY, deltaVetoLQTY);
+
+        (Governance.VoteSnapshot memory v, Governance.InitiativeVoteSnapshot memory initData) = governance.snapshotVotesForInitiative(address(maliciousInitiative2));
+        uint256 currentEpoch = governance.epoch();
+        assertEq(initData.lastCountedEpoch, currentEpoch - 1, "Epoch Matches");
+
+        // Inactive for 4 epochs
+        // Add another proposal
+
+        vm.warp(block.timestamp + governance.EPOCH_DURATION() * 4);
+        (v, initData) = governance.snapshotVotesForInitiative(address(maliciousInitiative2));
+        assertEq(initData.lastCountedEpoch, currentEpoch - 1, "Epoch Matches"); /// @audit This fails if you have 0 votes, see QA
+
+        uint256 unregisterSnapshot = vm.snapshot();
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.THROW);
+        governance.unregisterInitiative(address(maliciousInitiative2));
+        vm.revertTo(unregisterSnapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.OOG);
+        governance.unregisterInitiative(address(maliciousInitiative2));
+        vm.revertTo(unregisterSnapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.RETURN_BOMB);
+        governance.unregisterInitiative(address(maliciousInitiative2));
+        vm.revertTo(unregisterSnapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.REVERT_BOMB);
+        governance.unregisterInitiative(address(maliciousInitiative2));
+        vm.revertTo(unregisterSnapshot);
+
+        maliciousInitiative2.setRevertBehaviour(MaliciousInitiative.FunctionType.UNREGISTER, MaliciousInitiative.RevertType.NONE);
+        governance.unregisterInitiative(address(maliciousInitiative2));
+
+        governance.unregisterInitiative(address(eoaInitiative));
+    }
+
+
+
+}