Add audit fixes

[SoraSuegami]

No description provided.

[JohnGuilding]

Thanks for raising this @SoraSuegami. A few additional comments:

1. Looks like a few Safe tests are failing - I haven't looked into these as part of this review as ran out of time.

2. We notified zellic of an existing bug in the pre audit questionnaire - “Bug from not checking if module is installed in configureRecovery”. They agreed this needs fixing - we just need to add a check that previously existed - dm'd you with the message link

3. Based on these discussions from audit chat:

we need a function that anyone to cancel an ongoing recovery request after some time period for the case that a hash of the invalid argument is set when a wallet owner loses a private key.

Certainly, allowing anyone to cancel an ongoing recovery request after a certain period can prevent issues with improperly set hashes. However, I don't think this is the best practice, and I believe fixing the root cause would be better for improving code quality.

I think the contracts should have a function for anyone to cancel an ongoing recovery request after a certain period.
This is because 1) canceling the request keeps liveness even if we have any missing bug that causes the recovery fail and 2) it is difficult to prevent an invalid hash in the processRecovery function since it cannot access to recoveryData for that hash.
However, I agree that we add more validations of recoveryData and its hash.

a. Should we add a expiry time from when the recovery request is started? I think that sounds reasonable
b. and/or in addition to adding an expiry time, do you have any ideas on how we could fix the root cause of this issue? E.g. is the solution some extra validation (maybe it's an optional security check that developers can chose to add, to still provide out of the box recovery), is it offchain validation, is it do nothing etc

4. Do you think it is reasonable to do a rename of the contracts to account for body parsing being a feature in the upcoming months? e.g. the word subject is used a lot in the repo and we have contracts called SubjectHandlers. A rename is technically out of scope for audit fixes however, this naming could be misleading in the future. Maybe at least a rename of the subject handlers would be useful? wdyt?

[JohnGuilding]

For completeness even though risk is low, can you add unit tests for this new function?

You can just replicate EmailRecoveryManager_deInitRecoveryModule_Test, but for the new internal function with the account argument. You could even add tests in the same file.

Would need to update the harness to expose the internal function

[wshino]

Thank you, I've just added deInitRecoveryModuleWithAddress.t.sol.

[JohnGuilding]

Nitpick:

Don't think this minor duplication matters, but fyi test_Constructor_RevertWhen_SafeNotAllowedSelector is just replicating the same logic as test_Constructor_RevertWhen_UnsafeOnInstallSelector, as the second function still gets called when the account type is Safe. Same for test_AllowValidatorRecovery_RevertWhen_SafeNotAllowedSelector in allowValidatorRecovery.t.sol

Happy to leave as is

[SoraSuegami]

@JohnGuilding

a. Should we add a expiry time from when the recovery request is started? I think that sounds reasonable

I think we can just remove a recovery request in the completeRecovery function even if recover(account, recoveryData) in that function returns an error, rather than introducing a new expiry time.
This approach minimizes necessary changes.

and/or in addition to adding an expiry time, do you have any ideas on how we could fix the root cause of this issue?

I think it is difficult to completely prevent an invalid recovery data since the processRecovery function only takes its hash.
We can add a view function to simulate that recovery function will success off-chain, but it is a low priority.

Do you think it is reasonable to do a rename of the contracts to account for body parsing being a feature in the upcoming months?

I renamed them in another branch!