Fixing BRA-01 from CertiK audit

openfort-xyz · Dec 18, 2023 · d38d49c · d38d49c
1 parent b9e652d
commit d38d49c
Show file tree

Hide file tree

Showing 3 changed files with 49 additions and 0 deletions.
diff --git a/contracts/core/base/BaseRecoverableAccount.sol b/contracts/core/base/BaseRecoverableAccount.sol
@@ -218,6 +218,7 @@ abstract contract BaseRecoverableAccount is BaseOpenfortAccount, Ownable2StepUpg
     function proposeGuardian(address _guardian) external onlyOwner {
         if (isLocked()) revert AccountLocked();
         if (owner() == _guardian) revert GuardianCannotBeOwner();
+        if (pendingOwner() == _guardian) revert GuardianCannotBeOwner();
         if (isGuardian(_guardian)) revert DuplicatedGuardian();
         if (_guardian == address(0)) revert ZeroAddressNotAllowed();
 

diff --git a/test/foundry/core/managed/ManagedOpenfortAccountTest.t.sol b/test/foundry/core/managed/ManagedOpenfortAccountTest.t.sol
@@ -2506,4 +2506,28 @@ contract ManagedOpenfortAccountTest is OpenfortBaseTest {
         assertTrue(account.supportsInterface(type(IERC165).interfaceId));
         assertFalse(account.supportsInterface(bytes4(0x0000)));
     }
+
+    /*
+     * 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 {
+        ManagedOpenfortAccount openfortAccount = ManagedOpenfortAccount(payable(accountAddress));
+
+        address newOwner = makeAddr("newOwner");
+        vm.prank(openfortAdmin);
+        openfortAccount.transferOwnership(newOwner);
+
+        vm.prank(openfortAdmin);
+        openfortAccount.proposeGuardian(newOwner);
+
+        skip(SECURITY_PERIOD);
+        openfortAccount.confirmGuardianProposal(newOwner);
+
+        vm.prank(newOwner);
+        openfortAccount.acceptOwnership();
+
+        assertEq(openfortAccount.isGuardian(newOwner), true);
+        assertEq(openfortAccount.owner(), newOwner);
+    }
 }
diff --git a/test/foundry/core/upgradeable/UpgradeableOpenfortAccountTest.t.sol b/test/foundry/core/upgradeable/UpgradeableOpenfortAccountTest.t.sol
@@ -2844,4 +2844,28 @@ contract UpgradeableOpenfortAccountTest is OpenfortBaseTest {
         vm.prank(openfortAdmin);
         openfortFactory.updateInitialGuardian(address(openfortAdmin));
     }
+
+    /*
+     * 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 {
+        IBaseRecoverableAccount openfortAccount = IBaseRecoverableAccount(payable(accountAddress));
+
+        address newOwner = makeAddr("newOwner");
+        vm.prank(openfortAdmin);
+        openfortAccount.transferOwnership(newOwner);
+
+        vm.prank(openfortAdmin);
+        openfortAccount.proposeGuardian(newOwner);
+
+        skip(SECURITY_PERIOD);
+        openfortAccount.confirmGuardianProposal(newOwner);
+
+        vm.prank(newOwner);
+        openfortAccount.acceptOwnership();
+
+        assertEq(openfortAccount.isGuardian(newOwner), true);
+        assertEq(openfortAccount.owner(), newOwner);
+    }
 }