Feature/improve access control (#32)

* adding sepolia var file * improving security by checking msg.sender is a known contract for Fund Admin check * updating cash engine * updating core physical * chore: add tests for engine access reverts when caller has no role * updating logixc slightly * snapshot * chore: add more tests * adding registration scripts --------- Co-authored-by: Vitaly Galaichuk <swissarmytowel@gmail.com>
grappafinance · Apr 4, 2024 · b13a6e4 · b13a6e4
1 parent 8496350
commit b13a6e4
Show file tree

Hide file tree

Showing 12 changed files with 810 additions and 527 deletions.
diff --git a/.gas-snapshot b/.gas-snapshot
diff --git a/lib/core-cash b/lib/core-cash
diff --git a/lib/core-physical b/lib/core-physical
diff --git a/lib/entitlements b/lib/entitlements
diff --git a/script/.sepolia b/script/.sepolia
@@ -0,0 +1,38 @@
+Pomace=0xEC18EA2E88C3b2e992f091184FD458B51d632236
+PomaceProxy=0xBffcf4487728ADFF960c4Ac6565B257D5787E26F
+PomaceOptionTokenDescriptor=0xEC6a6c3ADaEA6274D74ba87edE29588Dae270B70
+PomaceOptionTokenDescriptorProxy=0xC1553c08013702cB17f96E553705161C36770cD9
+PomaceOptionToken=0xF25E64aD97D7Fb6eB89AaCF9A090C22880A0F211
+
+Grappa=0x239332f038FD35EFd09ae99133D868d8eDaF7353
+GrappaProxy=0x3748B3b3BAffBCbcD34cA9FF92716629a088558c
+GrappaOptionTokenDescriptor=0x93cD13050f85CE5FAAa83f2F8Ada484a9A7e1C4b.
+GrappaOptionTokenDescriptorProxy=0xcC9389c5FA74Ac4eD819Ae126D3fF7AEaeDF6575
+GrappaOptionToken=0x6772F41AD729ecBF43A57D4F48CeA7c8D13F947f
+
+CrossMarginPhysicalLib=0x926d3073Faa85ffC94D7d39272b97418F9130772.
+CrossMarginPhysicalMath=0xf1A77ad15119e0650110C6ec4F2F563AF9d17Ee0.
+CrossMarginPhysicalEngine=0xFdD68DA754360143DC99D797071B46143b8b1C86
+CrossMarginPhysicalEngineProxy=0xc1574ec6974dbb5B33212CaFF15e604dB2283289
+
+CrossMarginCashOracle=0xe15e96F69400Dc68a0354c1C73aa93f4BC7FB602
+CrossMarginCashLib=0x368456E86395d694402E0C658B380eD621DAB206.
+CrossMarginCashMath=0x950eb8E43D79549E8f431a58B1DBf5faaA881778.
+CrossMarginCashEngine=0x3F599fe37d8936F6855b22386e776a6110DA6144
+CrossMarginCashEngineProxy=0x1eE28580fE2377726017fC910513a37b792dCC9d
+
+CrossMarginOwner=0x8250e8c28178FDC736EC6e0258Cb26d4A8107F13
+RolesAuthorityProxy=0xFA4400c1B9AC496d9578B4B6507295A5aaD29EE7
+
+USDC=0x1c7D4B196Cb0C7B01d743Fbc6116a902379C7238
+WETH=0x8F49CEE1b150967B09F765414C1B40314bEFF0c5
+WBTC=0xEf4A6c5F9c1302CE8feee597e0Df38764534Bad1
+USYC=0x38D3A3f8717F4DB1CcB4Ad7D8C755919440848A3
+hnBTC=0xBcC728bf13718cF5dd1945fC1fFD8aAE2Ceb4D0f
+hnADA=0x393481E14961c700d16e571396b966E3f4Ca6EC8
+hnWSTETH=
+hnZRX=
+hnMATIC=
+MATIC=
+hnETH=
+hnBTC_Anchorage_TO=0x94ea8DcA053c123650FdB22d7808BDE28f609D39
diff --git a/script/register-assets.sol b/script/register-assets.sol
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import "forge-std/Script.sol";
+
+import "../src/settled-physical/CrossMarginPhysicalEngine.sol";
+
+interface IOptionProtocol {
+    function registerAsset(address asset) external returns (uint8);
+}
+
+contract RegisterAssets is Script {
+    function run() external {
+        console.log("Deployer", msg.sender);
+
+        vm.startBroadcast();
+
+        IOptionProtocol protocol = IOptionProtocol(vm.envAddress(""));
+
+        uint8 usdcId = protocol.registerAsset(vm.envAddress("USDC"));
+        console.log("USDC: \t\t", vm.envAddress("USDC"));
+        console.log("   -> Registered ID:", usdcId);
+
+        uint8 wethId = protocol.registerAsset(vm.envAddress("WETH"));
+        console.log("WETH: \t\t", vm.envAddress("WETH"));
+        console.log("   -> Registered ID:", wethId);
+
+        uint8 wbtcId = protocol.registerAsset(vm.envAddress("WBTC"));
+        console.log("WBTC: \t\t", vm.envAddress("WBTC"));
+        console.log("   -> Registered ID:", wbtcId);
+
+        uint8 usycId = protocol.registerAsset(vm.envAddress("USYC"));
+        console.log("USYC: \t\t", vm.envAddress("USYC"));
+        console.log("   -> Registered ID:", usycId);
+
+        uint8 hnBTCId = protocol.registerAsset(vm.envAddress("hnBTC"));
+        console.log("hnBTC: \t\t", vm.envAddress("hnBTC"));
+        console.log("   -> Registered ID:", hnBTCId);
+
+        uint8 hnADAId = protocol.registerAsset(vm.envAddress("hnADA"));
+        console.log("hnADA: \t\t", vm.envAddress("hnADA"));
+        console.log("   -> Registered ID:", hnADAId);
+
+        uint8 hnBTC_Anchorage_TOId = protocol.registerAsset(vm.envAddress("hnBTC_Anchorage_TO"));
+        console.log("hnBTC_Anchorage_TO: \t", vm.envAddress("hnBTC_Anchorage_TO"));
+        console.log("   -> Registered ID:", hnBTC_Anchorage_TOId);
+
+        // uint8 hnWSTETHId = protocol.registerAsset(vm.envAddress("hnWSTETH"));
+        // console.log("hnWSTETH: \t\t", vm.envAddress("hnWSTETH"));
+        // console.log("   -> Registered ID:", hnWSTETHId);
+
+        // uint8 hnZRXId = protocol.registerAsset(vm.envAddress("hnZRX"));
+        // console.log("hnZRX: \t\t", vm.envAddress("hnZRX"));
+        // console.log("   -> Registered ID:", hnZRXId);
+
+        // uint8 hnMATICId = protocol.registerAsset(vm.envAddress("hnMATIC"));
+        // console.log("hnMATIC: \t\t", vm.envAddress("hnMATIC"));
+        // console.log("   -> Registered ID:", hnMATICId);
+
+        // uint8 maticId = protocol.registerAsset(vm.envAddress("MATIC"));
+        // console.log("MATIC: \t\t", vm.envAddress("MATIC"));
+        // console.log("   -> Registered ID:", maticId);
+
+        // uint8 hnETHId = protocol.registerAsset(vm.envAddress("hnETH"));
+        // console.log("hnETH: \t\t", vm.envAddress("hnETH"));
+        // console.log("   -> Registered ID:", hnETHId);
+
+        vm.stopBroadcast();
+    }
+}
diff --git a/script/register-collateralizable.sol b/script/register-collateralizable.sol
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import "forge-std/Script.sol";
+
+import "../src/settled-physical/CrossMarginPhysicalEngine.sol";
+
+
+interface IContract {
+    function setCollateralizable(address _asset0, address _asset1, bool _value) external;
+}
+
+
+contract RegisterCollateralizable is Script {
+    function run() external {
+        console.log("Deployer", msg.sender);
+
+        vm.startBroadcast();
+
+        IContract app = IContract(vm.envAddress("CrossMarginCashEngineProxy"));
+        app.setCollateralizable(vm.envAddress("USDC"), vm.envAddress("USYC"), true);
+
+        app = IContract(vm.envAddress("PomaceProxy"));
+        app.setCollateralizable(vm.envAddress("USDC"), vm.envAddress("USYC"), true);
+
+        vm.stopBroadcast();
+    }
+}
diff --git a/script/register-engines.sol b/script/register-engines.sol
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import "forge-std/Script.sol";
+
+import "../src/settled-physical/CrossMarginPhysicalEngine.sol";
+
+
+interface IContract {
+    function registerEngine(address _engine) external returns (uint8);
+}
+
+
+contract RegisterCollateralizable is Script {
+    function run() external {
+        console.log("Deployer", msg.sender);
+
+        vm.startBroadcast();
+
+        IContract app = IContract(vm.envAddress("GrappaProxy"));
+
+        uint8 engineId = app.registerEngine(vm.envAddress("CrossMarginCashEngineProxy"));
+        console.log("CrossMarginCashEngine: \t\t", vm.envAddress("CrossMarginCashEngineProxy"));
+        console.log("   -> Registered ID:", engineId);
+
+        app = IContract(vm.envAddress("PomaceProxy"));
+
+        engineId = app.registerEngine(vm.envAddress("CrossMarginPhysicalEngineProxy"));
+        console.log("CrossMarginPhysicalEngine: \t\t", vm.envAddress("CrossMarginPhysicalEngineProxy"));
+        console.log("   -> Registered ID:", engineId);
+
+        vm.stopBroadcast();
+    }
+}
diff --git a/src/settled-cash/CrossMarginCashEngine.sol b/src/settled-cash/CrossMarginCashEngine.sol
@@ -566,9 +566,14 @@ contract CrossMarginCashEngine is
     function _assertCallerHasAccess(address _subAccount) internal override {
         if (_isPrimaryAccountFor(msg.sender, _subAccount)) return;
 
-        if (!authority.doesUserHaveRole(tx.origin, Role.System_FundAdmin)) {
-            super._assertCallerHasAccess(_subAccount);
+        if (
+            authority.doesUserHaveRole(tx.origin, Role.System_FundAdmin)
+                && (msg.sender == tx.origin || authority.getUserRoles(msg.sender) != bytes32(0))
+        ) {
+            return;
         }
+
+        super._assertCallerHasAccess(_subAccount);
     }
 
     function _computeDomainSeparator() internal view returns (bytes32) {

diff --git a/src/settled-physical/CrossMarginPhysicalEngine.sol b/src/settled-physical/CrossMarginPhysicalEngine.sol
@@ -672,9 +672,14 @@ contract CrossMarginPhysicalEngine is
     function _assertCallerHasAccess(address _subAccount) internal override {
         if (_isPrimaryAccountFor(msg.sender, _subAccount)) return;
 
-        if (!authority.doesUserHaveRole(tx.origin, Role.System_FundAdmin)) {
-            super._assertCallerHasAccess(_subAccount);
+        if (
+            authority.doesUserHaveRole(tx.origin, Role.System_FundAdmin)
+                && (msg.sender == tx.origin || authority.getUserRoles(msg.sender) != bytes32(0))
+        ) {
+            return;
         }
+
+        super._assertCallerHasAccess(_subAccount);
     }
 
     /**

diff --git a/test/unit-tests/AccountAccessCash.t.sol b/test/unit-tests/AccountAccessCash.t.sol
@@ -4,7 +4,7 @@ pragma solidity ^0.8.0;
 import "forge-std/Test.sol";
 
 // import test base and helpers.
-import {CrossMarginCashFixture} from "../integrations-cash/CrossMarginCashFixture.t.sol";
+import {CrossMarginCashFixture, Role} from "../integrations-cash/CrossMarginCashFixture.t.sol";
 
 import "grappa/config/types.sol";
 import "grappa/config/errors.sol";
@@ -66,6 +66,52 @@ contract CrossMarginCashEngineAccessTest is CrossMarginCashFixture {
         vm.stopPrank();
     }
 
+    function testCanAccessAccountIfOriginIsFundAdmin() public {
+        rolesAuthority.setUserRole(tx.origin, Role.System_FundAdmin, true);
+        _assertCanAccessAccount(alice, true);
+    }
+
+    function testCanAccessAccountIfOriginIsSender() public {
+        rolesAuthority.setUserRole(address(this), Role.System_FundAdmin, true);
+        // add capability for fund admin to run execute
+        rolesAuthority.setRoleCapability(Role.System_FundAdmin, address(engine), engine.execute.selector, true);
+        // remove the other role to make sure that only System_FundAdmin is present
+        rolesAuthority.setUserRole(address(this), Role.Investor_MFFeederDomestic, false);
+
+        // prank for address(this) is necessary to set tx.origin to address(this)
+        // by default tx.origin is DefaultSender address which is not the same as address(this)
+        vm.startPrank(address(this), address(this));
+        _assertCanAccessAccount(alice, true);
+        vm.stopPrank();
+    }
+
+    function testCannotAccessAccountIfOriginIsSenderNotFA() public {
+        // prank for address(this) is necessary to set tx.origin to address(this)
+        // by default tx.origin is DefaultSender address which is not the same as address(this)
+        vm.startPrank(address(this), address(this));
+        _assertCanAccessAccount(alice, false);
+        vm.stopPrank();
+    }
+
+    function testCannotAccessAccountIfOriginIsNotAdmin() public {
+        assertEq(rolesAuthority.doesUserHaveRole(tx.origin, Role.System_FundAdmin), false);
+        _assertCanAccessAccount(alice, false);
+    }
+
+    function testCannotAccessAccountIfCallerHasNoRole() public {
+        rolesAuthority.setUserRole(tx.origin, Role.System_FundAdmin, true);
+        _assertCanAccessAccount(alice, true);
+
+        // remove role and perform sanity check that it was removed and no other roles are present
+        rolesAuthority.setUserRole(address(this), Role.Investor_MFFeederDomestic, false);
+
+        assertEq(rolesAuthority.doesUserHaveRole(address(this), Role.Investor_MFFeederDomestic), false);
+        assertEq(rolesAuthority.getUserRoles(address(this)), bytes32(0));
+
+        // now caller should not be able to access the account
+        _assertCanAccessAccount(alice, false);
+    }
+
     function _assertCanAccessAccount(address subAccountId, bool _canAccess) internal {
         // we can update the account now
         ActionArgs[] memory actions = new ActionArgs[](1);

diff --git a/test/unit-tests/AccountAccessPhysical.t.sol b/test/unit-tests/AccountAccessPhysical.t.sol
@@ -4,7 +4,7 @@ pragma solidity ^0.8.0;
 import "forge-std/Test.sol";
 
 // import test base and helpers.
-import {CrossMarginPhysicalFixture} from "../integrations-physical/CrossMarginPhysicalFixture.t.sol";
+import {CrossMarginPhysicalFixture, Role} from "../integrations-physical/CrossMarginPhysicalFixture.t.sol";
 
 import {ActionArgs} from "pomace/config/types.sol";
 import "pomace/config/errors.sol";
@@ -66,6 +66,53 @@ contract CrossMarginPhysicalEngineAccessTest is CrossMarginPhysicalFixture {
         vm.stopPrank();
     }
 
+    function testCanAccessAccountIfOriginIsFundAdmin() public {
+        rolesAuthority.setUserRole(tx.origin, Role.System_FundAdmin, true);
+        _assertCanAccessAccount(alice, true);
+    }
+
+    function testCanAccessAccountIfOriginIsSender() public {
+        rolesAuthority.setUserRole(address(this), Role.System_FundAdmin, true);
+        // add capability for fund admin to run execute
+        rolesAuthority.setRoleCapability(Role.System_FundAdmin, address(engine), engine.execute.selector, true);
+        // remove the other role to make sure that only System_FundAdmin is present
+        rolesAuthority.setUserRole(address(this), Role.Investor_MFFeederDomestic, false);
+
+        // prank for address(this) is necessary to set tx.origin to address(this)
+        // by default tx.origin is DefaultSender address which is not the same as address(this)
+        vm.startPrank(address(this), address(this));
+        _assertCanAccessAccount(alice, true);
+        vm.stopPrank();
+    }
+
+    function testCannotAccessAccountIfOriginIsSenderNotFA() public {
+        // prank for address(this) is necessary to set tx.origin to address(this)
+        // by default tx.origin is DefaultSender address which is not the same as address(this)
+        vm.startPrank(address(this), address(this));
+        _assertCanAccessAccount(alice, false);
+        vm.stopPrank();
+    }
+
+    function testCannotAccessAccountIfOriginIsNotAdmin() public {
+        assertEq(rolesAuthority.doesUserHaveRole(tx.origin, Role.System_FundAdmin), false);
+        _assertCanAccessAccount(alice, false);
+    }
+
+    function testCannotAccessAccountIfCallerHasNoRole() public {
+        rolesAuthority.setUserRole(tx.origin, Role.System_FundAdmin, true);
+
+        _assertCanAccessAccount(alice, true);
+
+        // remove role and perform sanity check that it was removed and no other roles are present
+        rolesAuthority.setUserRole(address(this), Role.Investor_MFFeederDomestic, false);
+
+        assertEq(rolesAuthority.doesUserHaveRole(address(this), Role.Investor_MFFeederDomestic), false);
+        assertEq(rolesAuthority.getUserRoles(address(this)), bytes32(0));
+
+        // now caller should not be able to access the account
+        _assertCanAccessAccount(alice, false);
+    }
+
     function _assertCanAccessAccount(address subAccountId, bool _canAccess) internal {
         // we can update the account now
         ActionArgs[] memory actions = new ActionArgs[](1);