fix: Resolve security validation vulnerabilities in ColdStorageAddressBookModule (#111, #112)#113
Open
addnad wants to merge 1 commit into
Open
Conversation
…sBookModule (circlefin#111, circlefin#112) Issue circlefin#111: Fix unauthorized allowlist expansion - Problem: addAllowedRecipients() had skipRuntimeValidation: true, allowing anyone to expand any account's allowlist without authorization - Solution: Changed to skipRuntimeValidation: false and allowGlobalValidation: true for consistency with removeAllowedRecipients() - Impact: Now requires proper validation before allowing list modifications Issue circlefin#112: Enable intended execution module function calls - Problem: preRuntimeValidationHook and preUserOpValidationHook both reverted with Unsupported() at the end, preventing direct calls to addAllowedRecipients and removeAllowedRecipients - Solution: Changed revert Unsupported() to successful returns (return;/ return SIG_VALIDATION_SUCCEEDED;) to allow execution module functions to work - Impact: Execution module functions can now be called as intended Security Impact: - Prevents unauthorized modification of allowlists - Ensures only account owners can add/remove allowed recipients - Maintains consistency between execution functions' validation requirements Fixes: circlefin#111 circlefin#112
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes two critical security vulnerabilities in the ColdStorageAddressBookModule that could allow unauthorized access to account allowlists.
Issues Fixed
Issue #111: Unauthorized Allowlist Expansion
Vulnerability: The
addAllowedRecipients()function hadskipRuntimeValidation: true, allowing any external address to call it and expand an account's allowlist without authorization.Attack Flow:
account.addAllowedRecipients([attackerAddress])Fix: Changed to
skipRuntimeValidation: false, allowGlobalValidation: truefor consistency withremoveAllowedRecipients()Issue #112: Blocked Execution Module Function Calls
Problem: The validation hooks (
preRuntimeValidationHookandpreUserOpValidationHook) reverted withUnsupported()for any call not explicitly handled, preventing direct invocation of module execution functions.Impact: Users couldn't call
addAllowedRecipients()andremoveAllowedRecipients()as intended.Fix: Changed final reverts to successful returns, allowing execution module functions to work as designed.
Changes
skipRuntimeValidation: true→falsefor addAllowedRecipientsallowGlobalValidation: false→truefor addAllowedRecipientsrevert Unsupported()from validation hooks (replaced with returns)Security Impact
Fixes #111 #112