Single-step ownership transfer mechanism by `OwnableUpgradeable`

[hats-bug-reporter[bot]]

Github username: @Al-Qa-qa Twitter username: al_qa_qa Submission hash (on-chain): 0x684110b09bc05eb03565e9eef8d40a997ce274a3fc07fe01ea260b7c9d1a524b Severity: low

Description: Description\ In AtomWallet.sol the contract is an upgradable contract, but it uses single-step ownership transfer, this is not ideal for protocols where it can leave the contract without owner if it transfer the ownership to a wrong address.

AtomWallet.sol#L19

contract AtomWallet is Initializable, BaseAccount, OwnableUpgradeable { ... }

Single Step ownership transfer is dangerous as if the transfer is made to an incorrect address. the contract will be with no owner, and the role will be lost forever.

This will make the contract non-upgradable, where the owner is the only one who can upgrade the implementation of the VaultManger.

Recommendation

Use OZ::Ownable2StepUpgradeable.sol instead of single step ownership transfer.

[Al-Qa-qa]

We shoulld keep into consideration that the likelihood of transfereing ownership is HIGH in this protocol.

Where AtomWallets will be owned by Intuition when creating, then the ownership will get transferred to the owner of the atomURI if he can prove it.

And since we are creating more than one atomURI expressing Data in IPFS for example. changing ownership will occur more than one. which increases the likelihood of the Problem.

So in brief, this is not a simple ownership transfer that contract a contract that may get fired only one or two times in the hole life of he protocol. but it will get fired frequently, and there can be incorrect inputs from either users trying to claim the ownership of the wallet themselves, or the protocol.

Having 2step process will save the ownership of the wallet, by letting it to Intuition in case the owner address written incorrectly.

So I think as the likelihood increases, I think the issue lies in Medium/Low boundary not a LOW boundary. But this is better to be discussed in Judging process.

[mihailo-maksa]

The reported issue regarding the use of a single-step ownership transfer mechanism in AtomWallet.sol has been reviewed. Here is our detailed perspective:

Enhancement Suggestion: The suggestion to use Ownable2StepUpgradeable.sol instead of a single-step ownership transfer is a valid enhancement to improve the robustness of the contract. This change would mitigate the risk of the contract being left without an owner due to an incorrect address being provided during the ownership transfer.

Likelihood and Context: Given that ownership transfers in this protocol are expected to occur frequently, the likelihood of an error during the process is higher. Implementing a two-step transfer process would add an additional layer of security, ensuring that ownership is not accidentally lost. However, we should also consider that there are 2 scenarios for ownership transfer and they are as follows:

1. atomWarden gives ownership to the user who is determined to be the rightful owner of a particular AtomWallet, e.g. Vitalik's address becomes an owner of the AtomWallet representing the atom vault using "Vitalik Buterin" as the atomUri. In this case, since only atomWarden can perform this transfer of ownership, risks are significantly lower than they otherwise would've been.
2. User transfers ownership over an AtomWallet from their address to some other address, after atomWarden has awarded ownership to them. We believe this is the primary case where the lack of 2 step ownership transfer can pose certain risks, and this is the situation in which the reporter's enhancement suggestion can be considered to be the most valuable.

Severity Assessment: This issue does not constitute a security vulnerability or bug but rather a suggestion for improving the contract's design. The potential impact is limited to user experience and administrative control, rather than posing any direct security threat or financial risk.

Conclusion: While the recommendation to adopt a two-step ownership transfer process is a useful enhancement, it does not qualify as a security vulnerability. Therefore, we consider this issue to be an enhancement rather than a bug.

Status: This issue is invalid, but can be considered as an enhancement.

Comment for the Reporter: Thank you for the enhancement suggestion. We agree that implementing a two-step ownership transfer process would improve the contract's robustness and mitigate the risk of accidental ownership loss. However, since this does not pose a direct security vulnerability, we classify it as an enhancement. We can still consider a lower payout for this valid suggestion.

[Al-Qa-qa]

Hi @mihailo-maksa,

SingleStep Ownership transfer issue is considered LOW, and was considered MEDIUM before that, [1, 2, 3]

And I want to add that OwnerShip transfer differs a little bit in this protocol.

In most cases, OwnerShip is considered the controller of the protocol, and changing it may not happen or happen one time or something. Besides that, the persons who will make the process are devs, which will check the process carefully.

But in this protocol, the ownership of AtomWallet is desired to be transferred to anyone who proved the ownership of the data (atomURI), so there will be more than one transferring process, which increase the likelihood of the issue.

And the process depends on both, the user who will provide the address, and the protocol who will execute the tx. and user make a mistake when copying his address can occur as they are not technical, and may do mistakes.

In brief, I see the issue is valid and satisfied LOW at least.

[mihailo-maksa]

We consider this to be a minor issue, especially based on the two points outlined above, so this issue can still receive some lower payout.