Ackee audit fixes #22
Conversation
UniversalEmailRecoveryModule
…niversalEmailRecovery
EmailRecoveryManager
UniversalEmailRecoveryModule
| @@ -225,8 +234,8 @@ contract EmailRecoveryManager is EmailAccountRecovery, Initializable, IEmailReco | |||
| * @param expiry The expiry time after which the recovery attempt is invalid | |||
| */ | |||
| function configureRecovery( | |||
| address[] memory guardians, | |||
| uint256[] memory weights, | |||
| address[] calldata guardians, | |||
There was a problem hiding this comment.
If there are no changes for guardians, calldata is more appropriate than memory. Is that the reason this change?
There was a problem hiding this comment.
yeah, that is why they are now calldata?
Although I change them back in the consolidation PR as the function is called directly by the module. As they are abi decoded in onInstall, this means they are in memory so these params have to be changed back to memory. That is for the other PR though
| uint256 guardianCount; // total count for all guardians | ||
| uint256 totalWeight; // combined weight for all guardians. Important for checking that | ||
| // thresholds are valid. | ||
| uint256 acceptedWeight; // combined weight for all accepted guardians. This is separated |
| if (recoveryRequest.currentWeight >= threshold) { | ||
| if (recoveryRequest.currentWeight >= guardianConfig.threshold) { | ||
| bytes32 calldataHash = IEmailRecoverySubjectHandler(subjectHandler) | ||
| .parseRecoveryCalldataHash(templateIdx, subjectParams); |
There was a problem hiding this comment.
Was this function separated from validateRecoverySubject to return calldataHash efficiently?
There was a problem hiding this comment.
Yeah it was a recommendation from the auditors to reduce unnecessary computation every time processRecovery was called
| if ( | ||
| _selector == IModule.onUninstall.selector || _selector == IModule.onInstall.selector | ||
| || _selector == IERC7579Account.execute.selector | ||
| || _selector == ISafe.setFallbackHandler.selector |
There was a problem hiding this comment.
Could you tell me why we need to check FallbackHandler.selector?
There was a problem hiding this comment.
It was a recommendation from the auditors. A 7579 Safe can be used as a validator. And that contract can execute arbitrary logic on a safe including the function - setFallbackHandler. Adding this check stops that function being called directly from the Safe on recovery as it could cause some damage if it was called
Notes:
H2: Premature guardian configuration update in addGuardian function
Decided to add
acceptedWeightas a dedicated variable, as the alternative was to loop over guardians every time processRecovery is called, which could be quite computationally expensive. Adding the dedicated variable didn’t actually add too much complexityM3: Selector collisions in UniversalEmailRecoveryModule
The simplest solution to this was to remove the
selectorToValidatormapping and just pass the validator in with the calldata torecover. I used assembly to retrieve the selector as therecoveryCalldatais now stored in memory and not in calldata since it is abi decoded along with the target validatorW4: Gas inefficiencies in UniversalRecoveryModule
Issue 1 was fixed by the function selector collision change
Issue 2 is resolved
Issue 3 -
getAllowedValidatorsneeded to be called anyway so didn’t implement - unless you see a way it isn’t needed we missedW6: Missing AddedGuardian event emission in setupGuardians function
Emitted the AddedGuardian event and simplified the code by just calling
addGuardianinsidesetupGuardiansW7: ERC-4337 violation in onInstall
This was a great catch. Resolved by adding comments explaining the violation as decided it was too complex to make it compatible. Future versions could look to resolve this
I6: Floating pragma
Chose not to implement for better compatibility with external contracts
I9: Safe validateRecoverySubject optimization
Didn’t need to combine with
getPreviousOwnerInLinkedListas that was moved into theparseRecoveryCalldatafunction. Did check thenewOwneragainst existing owners