Further fixing BRA-01 from CertiK audit

openfort-xyz · Dec 22, 2023 · 42724b8 · 42724b8
1 parent d72d7a6
commit 42724b8
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 1 deletion.
diff --git a/contracts/core/base/BaseRecoverableAccount.sol b/contracts/core/base/BaseRecoverableAccount.sol
@@ -420,6 +420,12 @@ abstract contract BaseRecoverableAccount is BaseOpenfortAccount, Ownable2StepUpg
     function transferOwnership(address _newOwner) public override {
         if (isLocked()) revert AccountLocked();
         if (isGuardian(_newOwner)) revert GuardianCannotBeOwner();
+        if (
+            guardiansConfig.info[_newOwner].pending != 0
+                && block.timestamp <= guardiansConfig.info[_newOwner].pending + securityWindow
+        ) {
+            revert GuardianCannotBeOwner();
+        }
         super.transferOwnership(_newOwner);
     }
 

diff --git a/test/foundry/core/managed/ManagedOpenfortAccountTest.t.sol b/test/foundry/core/managed/ManagedOpenfortAccountTest.t.sol
@@ -2524,7 +2524,7 @@ contract ManagedOpenfortAccountTest is OpenfortBaseTest {
      * Testcase where a pending owner is proposed as guardian. It used to work, should fail now.
      * From the CertiK audit, issue BRA-01
      */
-    function testFailAddPendingOwnerAsGuardian() public {
+    function testFailAddPendingOwnerAsGuardian1() public {
         ManagedOpenfortAccount openfortAccount = ManagedOpenfortAccount(payable(accountAddress));
 
         address newOwner = makeAddr("newOwner");
@@ -2543,4 +2543,27 @@ contract ManagedOpenfortAccountTest is OpenfortBaseTest {
         assertEq(openfortAccount.isGuardian(newOwner), true);
         assertEq(openfortAccount.owner(), newOwner);
     }
+
+    /*
+     * Testcase where a pending owner is proposed as guardian. It used to work, should fail now.
+     * From the CertiK audit, issue BRA-01
+     */
+    function testFailAddPendingOwnerAsGuardian2() public {
+        ManagedOpenfortAccount openfortAccount = ManagedOpenfortAccount(payable(accountAddress));
+        address newOwner = makeAddr("newOwner");
+
+        vm.prank(openfortAdmin);
+        openfortAccount.proposeGuardian(newOwner);
+
+        vm.prank(openfortAdmin);
+        openfortAccount.transferOwnership(newOwner);
+
+        skip(SECURITY_PERIOD);
+        openfortAccount.confirmGuardianProposal(newOwner);
+
+        vm.prank(newOwner);
+        openfortAccount.acceptOwnership();
+
+        assertEq(openfortAccount.isGuardian(newOwner), true);
+    }
 }