HAMT-37: Security context refactor

scripts/FullEscrowDeployment.s.sol

@@ -12,15 +12,16 @@ import { EligibilityModule } from "../src/hats/EligibilityModule.sol";
 import { ToggleModule } from "../src/hats/ToggleModule.sol";
 
 // Security context & system
-import { HatsSecurityContext } from "../src/HatsSecurityContext.sol";
-import { IHatsSecurityContext } from "../src/IHatsSecurityContext.sol";
+import { HatsSecurityContext } from "../src/security/HatsSecurityContext.sol";
+import { ISecurityContext } from "../src/security/ISecurityContext.sol";
 import { SystemSettings } from "../src/SystemSettings.sol";
+import { IPurchaseTracker } from "../src/IPurchaseTracker.sol";
 import { ISystemSettings } from "../src/ISystemSettings.sol";
 import { PaymentEscrow } from "../src/PaymentEscrow.sol";
 import {EscrowMulticall} from "../src/EscrowMulticall.sol";
 
 // Roles
-import { Roles } from "../src/Roles.sol";
+import { Roles } from "../src/security/Roles.sol";
 
 import "./HatsDeployment.s.sol";
 
@@ -54,11 +55,11 @@ contract FullEscrowDeployment is Script {
     // 8. Deploy SystemSettings             //
     //--------------------------------------//
     // The SystemSettings constructor requires:
-    //   IHatsSecurityContext
+    //   ISecurityContext
     //   vaultAddress
     //   initialFeeBps
     systemSettings = new SystemSettings(
-      IHatsSecurityContext(hatsSecurityContextAddr),
+      ISecurityContext(hatsSecurityContextAddr),
       safeAddr, //CHANGE FOR VAULT
       0 // feeBps (0 for now)
     );
@@ -67,13 +68,14 @@ contract FullEscrowDeployment is Script {
     // 9. Deploy PaymentEscrow              //
     //--------------------------------------//
     // PaymentEscrow requires:
-    //   IHatsSecurityContext
+    //   ISecurityContext
     //   ISystemSettings
     //   autoReleaseFlag
     paymentEscrow = new PaymentEscrow(
-      IHatsSecurityContext(hatsSecurityContextAddr),
+      ISecurityContext(hatsSecurityContextAddr),
       ISystemSettings(address(systemSettings)),
-      autoRelease
+      autoRelease, 
+      IPurchaseTracker(address(0))
     );
 
     // ----------------------//

scripts/HatsDeployment.s.sol

@@ -15,9 +15,8 @@ interface IGnosisSafe {
 import { Hats } from "@hats-protocol/Hats.sol";
 import { EligibilityModule } from "../src/hats/EligibilityModule.sol";
 import { ToggleModule } from "../src/hats/ToggleModule.sol";
-import { HatsSecurityContext } from "../src/HatsSecurityContext.sol";
-import { IHatsSecurityContext } from "../src/IHatsSecurityContext.sol";
-import { Roles } from "../src/Roles.sol";
+import { HatsSecurityContext } from "../src/security/HatsSecurityContext.sol";
+import { Roles } from "../src/security/Roles.sol";
 
 import "@gnosis.pm/safe-contracts/contracts/proxies/GnosisSafeProxyFactory.sol";
 

src/PaymentEscrow.sol

@@ -1,13 +1,13 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.20;
 
-import "./HasSecurityContext.sol";
+import "./security/HasSecurityContext.sol";
+import "./security/Roles.sol";
 import "./ISystemSettings.sol";
 import "./CarefulMath.sol";
 import "./PaymentInput.sol";
 import "./IEscrowContract.sol";
 import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
-import "./Roles.sol";
 import "./IPurchaseTracker.sol";
 
 /**
@@ -84,7 +84,7 @@ contract PaymentEscrow is HasSecurityContext, IEscrowContract
      * @param _purchaseTracker Address of the PurchaseTracker singleton.
      */
     constructor(
-        IHatsSecurityContext securityContext,
+        ISecurityContext securityContext,
         ISystemSettings settings_,
         bool autoRelease,
         IPurchaseTracker _purchaseTracker

src/SystemSettings.sol

@@ -1,9 +1,9 @@
 // SPDX-License-Identifier: UNLICENSED
 pragma solidity ^0.8.20;
 
-import "./HasSecurityContext.sol";
+import "./security/HasSecurityContext.sol";
+import "./security/Roles.sol";
 import "./ISystemSettings.sol";
-import "./Roles.sol";
 
 /**
  * @title SystemSettings
@@ -46,7 +46,7 @@ contract SystemSettings is HasSecurityContext, ISystemSettings {
      * @param vaultAddress_ Recipient of the extracted fees.
      * @param feeBps_ Amount of fees charged in basis points.
      */
-    constructor(IHatsSecurityContext securityContext, address vaultAddress_, uint256 feeBps_) {
+    constructor(ISecurityContext securityContext, address vaultAddress_, uint256 feeBps_) {
         _setSecurityContext(securityContext);
         if (vaultAddress_ == address(0)) {
             revert("InvalidVaultAddress");

src/HasSecurityContext.sol → src/security/HasSecurityContext.sol

@@ -9,7 +9,7 @@ import "./Roles.sol";
  * @title HasSecurityContext
  */
 abstract contract HasSecurityContext {
-    IHatsSecurityContext public securityContext;
+    ISecurityContext public securityContext;
 
     bool private initialized = false;
 
@@ -19,25 +19,23 @@ abstract contract HasSecurityContext {
     event SecurityContextSet(address indexed caller, address indexed securityContext);
 
     modifier onlyRole(bytes32 role) {
-        uint256 hatId = securityContext.roleToHatId(role);
-        if (!Hats(securityContext.hats()).isWearerOfHat(msg.sender, hatId)) {
+        if (!securityContext.hasRole(role, msg.sender)) {
             revert UnauthorizedAccess(role, msg.sender);
         }
         _;
     }
 
-    function setSecurityContext(IHatsSecurityContext _securityContext) external onlyRole(Roles.ADMIN_ROLE) {
+    function setSecurityContext(ISecurityContext _securityContext) external onlyRole(Roles.ADMIN_ROLE) {
         _setSecurityContext(_securityContext);
     }
 
-    function _setSecurityContext(IHatsSecurityContext _securityContext) internal {
+    function _setSecurityContext(ISecurityContext _securityContext) internal {
         if (address(_securityContext) == address(0)) revert ZeroAddressArgument();
 
         if (!initialized) {
             initialized = true;
         } else {
-            uint256 adminHatId = _securityContext.roleToHatId(Roles.ADMIN_ROLE);
-            require(_securityContext.hats().isWearerOfHat(msg.sender, adminHatId), "Caller is not admin");
+            require(_securityContext.hasRole(Roles.ADMIN_ROLE, msg.sender), "Caller is not admin");
         }
 
         if (securityContext != _securityContext) {

src/IHatsSecurityContext.sol → src/security/IHatsSecurityContext.sol

@@ -2,16 +2,9 @@
 pragma solidity ^0.8.20;
 
 import "@hats-protocol/Hats.sol";
+import "./ISecurityContext.sol";
 
-interface IHatsSecurityContext {
-    /**
-     * @notice Checks if an account has the specified role.
-     * @param role The role to query, identified by a `bytes32` role ID.
-     * @param account The address to check for the specified role.
-     * @return True if the account has the specified role, otherwise false.
-     */
-    function hasRole(bytes32 role, address account) external view returns (bool);
-
+interface IHatsSecurityContext is ISecurityContext {
     /**
      * @notice Returns the Hat ID associated with a specific role.
      * @param role The role identifier (as a `bytes32` value).

test-foundry/EscrowMulticallTest.t.sol

@@ -3,7 +3,7 @@ pragma solidity ^0.8.17;
 
 import "forge-std/Test.sol";
 import "../lib/hats-protocol/src/Hats.sol";
-import "../src/HatsSecurityContext.sol";
+import "../src/security/HatsSecurityContext.sol";
 import "../src/SystemSettings.sol";
 import "../src/PaymentEscrow.sol";
 import "../src/EscrowMulticall.sol";
@@ -13,7 +13,7 @@ import { Payment } from "../src/PaymentInput.sol";
 import { MulticallPaymentInput } from "../src/EscrowMulticall.sol";
 import { FailingToken } from "../src/FailingToken.sol";
 import { TestToken } from "../src/TestToken.sol";
-import { IHatsSecurityContext } from "../src/IHatsSecurityContext.sol";
+import { ISecurityContext } from "../src/security/ISecurityContext.sol";
 import "../src/hats/EligibilityModule.sol";
 import "../src/hats/ToggleModule.sol";
 import "../src/IPurchaseTracker.sol";
@@ -122,12 +122,12 @@ contract EscrowMulticallTest is Test {
         hats.mintHat(daoHatId, dao);
 
         testToken = new TestToken("XYZ", "ZYX");
-        systemSettings = new SystemSettings(IHatsSecurityContext(address(securityContext)), vaultAddress, 0);
+        systemSettings = new SystemSettings(ISecurityContext(address(securityContext)), vaultAddress, 0);
 
-        escrow = new PaymentEscrow(IHatsSecurityContext(address(securityContext)), ISystemSettings(address(systemSettings)), false, IPurchaseTracker(address(0)));
+        escrow = new PaymentEscrow(ISecurityContext(address(securityContext)), ISystemSettings(address(systemSettings)), false, IPurchaseTracker(address(0)));
         escrow1 = escrow;
-        escrow2 = new PaymentEscrow(IHatsSecurityContext(address(securityContext)), ISystemSettings(address(systemSettings)), false, IPurchaseTracker(address(0)));
-        escrow3 = new PaymentEscrow(IHatsSecurityContext(address(securityContext)), ISystemSettings(address(systemSettings)), false, IPurchaseTracker(address(0)));
+        escrow2 = new PaymentEscrow(ISecurityContext(address(securityContext)), ISystemSettings(address(systemSettings)), false, IPurchaseTracker(address(0)));
+        escrow3 = new PaymentEscrow(ISecurityContext(address(securityContext)), ISystemSettings(address(systemSettings)), false, IPurchaseTracker(address(0)));
 
         multicall = new EscrowMulticall();
 
@@ -192,7 +192,7 @@ contract EscrowMulticallTest is Test {
         return convertPayment(pm);
     }
 
-    function verifyPayment(Payment memory actual, Payment memory expected) internal {
+    function verifyPayment(Payment memory actual, Payment memory expected) internal pure {
         assertEq(actual.id, expected.id);
         assertEq(actual.payer, expected.payer);
         assertEq(actual.receiver, expected.receiver);
@@ -205,7 +205,7 @@ contract EscrowMulticallTest is Test {
     }
 
     // Deployment
-    function testDeploymentArbiterRole() public {
+    function testDeploymentArbiterRole() public view {
         bool hasArbiter = hats.isWearerOfHat(arbiter, arbiterHatId);
         bool hasNonOwnerArbiter = hats.isWearerOfHat(nonOwner, arbiterHatId);
         bool hasVaultArbiter = hats.isWearerOfHat(vaultAddress, arbiterHatId);

test-foundry/HatsTest.t.sol

@@ -4,11 +4,11 @@ pragma solidity ^0.8.19;
 import "forge-std/Test.sol";
 import "../lib/hats-protocol/src/Hats.sol";
 
-import { HatsSecurityContext } from "../src/HatsSecurityContext.sol";
+import { HatsSecurityContext } from "../src/security/HatsSecurityContext.sol";
 import { PaymentEscrow, IEscrowContract } from "../src/PaymentEscrow.sol";
 import { SystemSettings } from "../src/SystemSettings.sol";
 import { TestToken } from "../src/TestToken.sol";
-import { IHatsSecurityContext } from "../src/IHatsSecurityContext.sol";
+import { ISecurityContext } from "../src/security/ISecurityContext.sol";
 import { ISystemSettings } from "../src/ISystemSettings.sol";
 import { PaymentInput, Payment } from "../src/PaymentInput.sol";
 import { FailingToken } from "../src/FailingToken.sol";
@@ -198,14 +198,14 @@ contract PaymentEscrowHatsTest is Test {
         // 10. Deploy SystemSettings
         vm.startPrank(admin);
         systemSettings = new SystemSettings(
-            IHatsSecurityContext(address(hatsSecurityContext)),
+            ISecurityContext(address(hatsSecurityContext)),
             vault, // Vault address
             0 // Initial fee BPS
         );
 
         // 11. Deploy PaymentEscrow
         escrow = new PaymentEscrow(
-            IHatsSecurityContext(address(hatsSecurityContext)),
+            ISecurityContext(address(hatsSecurityContext)),
             ISystemSettings(address(systemSettings)),
             false, // autoReleaseFlag
             IPurchaseTracker(address(0)) 
@@ -399,7 +399,7 @@ contract PaymentEscrowHatsTest is Test {
         assertFalse(hats.isWearerOfHat(testAddress, testHatId), "Test address should not own the hat");
     }
 
-    function testToggleModule_DefaultsToActive() public {
+    function testToggleModule_DefaultsToActive() public view {
         bool arbiterIsActive = hats.isActive(arbiterHatId);
         assertTrue(arbiterIsActive, "Arbiter Hat should be active by default");
 

test-foundry/PaymentEscrowTest.t.sol

@@ -4,10 +4,10 @@ pragma solidity ^0.8.19;
 import "forge-std/Test.sol";
 import "../lib/hats-protocol/src/Hats.sol";
 import {PaymentEscrow, IEscrowContract} from "../src/PaymentEscrow.sol";
-import {HatsSecurityContext} from "../src/HatsSecurityContext.sol";
+import {HatsSecurityContext} from "../src/security/HatsSecurityContext.sol";
 import {SystemSettings} from "../src/SystemSettings.sol";
 import {TestToken} from "../src/TestToken.sol";
-import {IHatsSecurityContext} from "../src/IHatsSecurityContext.sol";
+import {ISecurityContext} from "../src/security/ISecurityContext.sol";
 import {ISystemSettings} from "../src/ISystemSettings.sol";
 import {PaymentInput, Payment} from "../src/PaymentInput.sol";
 import {console} from "forge-std/console.sol";
@@ -139,8 +139,8 @@ contract PaymentEscrowTest is Test {
         hats.mintHat(systemHatId, system);
 
         testToken = new TestToken("XYZ", "ZYX");
-        systemSettings = new SystemSettings(IHatsSecurityContext(address(securityContext)), vaultAddress, 0);
-        escrow = new PaymentEscrow(IHatsSecurityContext(address(securityContext)), ISystemSettings(address(systemSettings)), false, IPurchaseTracker(address(0)));
+        systemSettings = new SystemSettings(ISecurityContext(address(securityContext)), vaultAddress, 0);
+        escrow = new PaymentEscrow(ISecurityContext(address(securityContext)), ISystemSettings(address(systemSettings)), false, IPurchaseTracker(address(0)));
 
         testToken.mint(nonOwner, 10_000_000_000);
         testToken.mint(payer1, 10_000_000_000);
@@ -189,7 +189,7 @@ contract PaymentEscrowTest is Test {
     function _verifyPayment(
         Payment memory actual,
         Payment memory expected
-    ) internal {
+    ) internal pure {
         assertEq(actual.id, expected.id);
         assertEq(actual.payer, expected.payer);
         assertEq(actual.receiver, expected.receiver);
@@ -226,7 +226,7 @@ contract PaymentEscrowTest is Test {
         balanceSnapshot[4] = _getBalance(payer2, false);
     }
 
-    function _verifyBalanceChange(uint256 index, int256 expectedChange) internal {
+    function _verifyBalanceChange(uint256 index, int256 expectedChange) internal view {
         uint256 newBalance = index == 0 ? address(escrow).balance :
                            index == 1 ? _getBalance(receiver1, false) :
                            index == 2 ? _getBalance(payer1, false) :
@@ -241,7 +241,7 @@ contract PaymentEscrowTest is Test {
     }
 
     // Deployment
-    function testDeploymentArbiterRole() public {
+    function testDeploymentArbiterRole() public view {
         bool hasArbiterRole = hats.isWearerOfHat(arbiter, arbiterHatId);
         bool hasNonOwnerRole = hats.isWearerOfHat(nonOwner, arbiterHatId);
         bool hasVaultRole = hats.isWearerOfHat(vaultAddress, arbiterHatId);
@@ -886,7 +886,7 @@ contract PaymentEscrowTest is Test {
         // total refunded now = amount
     }
 
-    function testFailRefundAfterReleaseWithActiveEscrow() public {
+    function testRefundAfterReleaseWithActiveEscrowIsReverted() public {
         // Exploit: malcious actor is able to refund the payment after it has been released stealing the funds in escrow 
         // 1. Place a payment from payer1 to receiver1
         // 2. malicious actor pays self through escrow and releases the payment to self
@@ -930,6 +930,7 @@ contract PaymentEscrowTest is Test {
 
         // Malicious refund attempt by receiver2
         vm.prank(receiver2);
+        vm.expectRevert("Payment already released"); 
         escrow.refundPayment(paymentIdReleased, amountReleased);
 
         // Final balances after the malicious refund
@@ -944,6 +945,17 @@ contract PaymentEscrowTest is Test {
         // finalBalances[3] - Receiver2's balance
         // finalBalances[4] - Payer2's balance
 
+        assertTrue(
+            finalBalances[3] + finalBalances[4] == initialBalances[3] + initialBalances[4],
+            "Exploit success: Receiver2 profited"
+        );
+
+        assertTrue(
+            finalBalances[1] + finalBalances[2] + finalBalances[0] == initialBalances[1] + initialBalances[2] + initialBalances[0],
+            "Exploit success: Payer1 and Receiver1 lost funds"
+        );
+
+        /*
         // Assertions
         // Check if Receiver2 profited (exploit success)
         assertTrue(
@@ -959,6 +971,7 @@ contract PaymentEscrowTest is Test {
 
         // Ensure the escrow balance was drained to 0
         assertEq(finalBalances[0], 0, "Exploit success: Escrow balance drained");
+        */
     }
 
     function testCannotRefundAfterRelease() public {
@@ -1262,7 +1275,7 @@ contract PaymentEscrowTest is Test {
         assertEq(finalPayerBalance, initialPayerBalance);
     }
 
-    function testGetPaymentForNonExistentID() public {
+    function testGetPaymentForNonExistentID() public view {
         bytes32 nonExistentPaymentId = keccak256("non-existent-payment");
 
         Payment memory payment = escrow.getPayment(nonExistentPaymentId);
@@ -1614,7 +1627,7 @@ contract PaymentEscrowTest is Test {
 
         vm.startPrank(admin);
         PaymentEscrow escrowAutoRelease = new PaymentEscrow(
-            IHatsSecurityContext(address(securityContext)),
+            ISecurityContext(address(securityContext)),
             ISystemSettings(address(systemSettings)),
             true, // autoReleaseFlag = true
             IPurchaseTracker(address(0))
@@ -1625,7 +1638,7 @@ contract PaymentEscrowTest is Test {
         uint256 amount = 1 ether;
 
         // Record initial balances
-        uint256 payerInitialBalance = payer1.balance;
+        //uint256 payerInitialBalance = payer1.balance;
         uint256 receiverInitialBalance = receiver1.balance;
 
         // Place the payment in the new escrow
@@ -1915,4 +1928,4 @@ contract RevertingReceiver {
     }
     // test function so foundry ignores this contract
     function test() public pure {}
-}
+}