Failure of signature validation due to renounce of ownership

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x614eb3d24183cf703374120eac6ed9bd3a62b0b0ba295e9b609450b23948c288 Severity: low

Description: Description\

AtomWallet.sol has inherited openzeppelin's OwnableUpgradeable.sol contract for owner related functionalities. This also means that renounceOwnership() can be directly called and owner would be set to zero address. This high impact would affect the `AtomWallet.sol contract in following way:

    function _validateSignature(UserOperation calldata userOp, bytes32 userOpHash)
        internal
        virtual
        override
        returns (uint256 validationData)
    {
        bytes32 hash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", userOpHash));

        (address recovered,,) = ECDSA.tryRecover(hash, userOp.signature);

@>      if (recovered != owner()) {
            return SIG_VALIDATION_FAILED;
        }

        return 0;
    }

To validated the signature, the above function checks that the recovered address is indeed owner of wallet. However, when renounceOwnership() function is called by wallet owner then the check if (recovered != owner()) { return SIG_VALIDATION_FAILED; would fail and signature would not be validated.

Recommendations\ Disable the renounceOwnership() function by overriding it.

NOTE: This should not be considered centralized risk as the likelihood is low but the impact is very much high so considering the contest, it should be low severtiy.

[mihailo-maksa]

After careful review, we have determined that this issue is not a valid concern for the following reasons:

1. User Autonomy: The ability to renounce ownership is a feature that allows users to relinquish control over their wallet if they choose to do so. This decision should remain within the user's discretion.
2. Controlled Impact: When a user renounces ownership, they are aware that the contract's owner address will be set to zero. This is a conscious decision and does not inadvertently compromise the wallet's security or functionality.
3. Intended Behavior: The _validateSignature function's design is intentional. It ensures that only the owner can validate signatures. If the ownership is renounced, the contract behaves as expected by failing the signature validation, indicating no current owner.

In conclusion, while disabling the renounceOwnership() function might seem beneficial, it undermines user autonomy. Our implementation works as intended, allowing users to make their own choices about ownership. Therefore, this issue is not a valid concern, and we consider it invalid.

[0xRizwan]

@mihailo-maksa Thanks for the detailed comment.

This is actually a risk associated with users action and it is also not documented in shared Intuition documentation. In addition to above recommendation, i would suggest this issue risk to add in documentation or as a part of disclaimer, additionally, an alert can shown to users at front-end so that everyone including non-technical people can understand it. Therefore, this issue still can be considered as an enhancement or issue can be acknowledged.

[mihailo-maksa]

We understand your concerns, but we really want to emphasize the point that our implementation works as intended, allowing users to make their own choices about ownership. Also, we are planning on implementing the Ownable2StepUpgradeable in AtomWallet instead of OwnableUpgradeable, thus further addressing the security concerns our users might have, while also maintaining the ability to renounce the ownership, which we believe is crucial to our protocol.

[0xRizwan]

@cgmello Renounce of ownership in both Ownable2StepUpgradeable and OwnableUpgradeable is a single step process so using any of these versions wont fix this issue. This issue brings valid concern for users. I believe if #72 can be considered as an issue then this can be too. Thank you for the discussions on this issue, i respect your decision.

[mihailo-maksa]

The ability to renounce ownership is intentional and aligns with our design, making this issue invalid. The reason why #72 was accepted is that the fix was actually useful and implemented, whereas this issue does not necessitate a change.