Merge pull request #12 from openfort-xyz/BRA-03M_prevent_guardians_co…

…nfirmation_with_ongoin_recovery fix: prevent new guardian when recovery is ongoing
openfort-xyz · Jan 20, 2025 · 98b80bd · 98b80bd
2 parents 3f9fc1a + 47b7a7e
commit 98b80bd
Show file tree

Hide file tree

Showing 2 changed files with 11 additions and 1 deletion.
diff --git a/contracts/core/base/BaseRecoverableAccount.sol b/contracts/core/base/BaseRecoverableAccount.sol
@@ -239,6 +239,7 @@ abstract contract BaseRecoverableAccount is BaseOpenfortAccount, Ownable2StepUpg
      * @param _guardian The guardian to be confirmed.
      */
     function confirmGuardianProposal(address _guardian) external {
+        _requireRecovery(false);
         if (isLocked()) revert AccountLocked();
         if (guardiansConfig.info[_guardian].pending == 0) revert UnknownProposal();
         if (guardiansConfig.info[_guardian].pending > block.timestamp) revert PendingProposalNotOver();

diff --git a/test/foundry/core/upgradeable/UpgradeableOpenfortAccountTest.t.sol b/test/foundry/core/upgradeable/UpgradeableOpenfortAccountTest.t.sol
@@ -1534,6 +1534,7 @@ contract UpgradeableOpenfortAccountTest is OpenfortBaseTest {
 
         // Create a friend EOA
         address friendAccount = makeAddr("friend");
+        address badFriendAccount = makeAddr("bad friend");
 
         // Trying to propose a guardian not using the owner
         vm.expectRevert("Ownable: caller is not the owner");
@@ -1544,6 +1545,8 @@ contract UpgradeableOpenfortAccountTest is OpenfortBaseTest {
         emit GuardianProposed(friendAccount, block.timestamp + SECURITY_PERIOD);
         vm.prank(openfortAdmin);
         openfortAccount.proposeGuardian(friendAccount);
+        vm.prank(openfortAdmin);
+        openfortAccount.proposeGuardian(badFriendAccount);
 
         // Verify that the number of guardians is still 1 (default)
         assertEq(openfortAccount.guardianCount(), 1);
@@ -1561,12 +1564,18 @@ contract UpgradeableOpenfortAccountTest is OpenfortBaseTest {
 
         skip(SECURITY_PERIOD);
         openfortAccount.confirmGuardianProposal(friendAccount);
-
         // Verify that the number of guardians is now 2
         assertEq(openfortAccount.guardianCount(), 2);
 
         // Friend account should be a guardian now
         assertEq(openfortAccount.isGuardian(friendAccount), true);
+
+        vm.prank(friendAccount);
+        openfortAccount.startRecovery(makeAddr("recovery address"));
+
+        // Can't confirm a guardian proposal while recovery is ongoing
+        vm.expectRevert(OngoingRecovery.selector);
+        openfortAccount.confirmGuardianProposal(badFriendAccount);
     }
 
     /*