hats-finance / Intuition-0x538dbadc50cc87b281cd655f1edbc6ebda02a66a

The smart contracts of the Intuition protocol v1.
https://intuition.systems
Other
0 stars 1 forks source link

wallet ownership transfer should be 2 step transfer #16

Open hats-bug-reporter[bot] opened 2 weeks ago

hats-bug-reporter[bot] commented 2 weeks ago

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

Description: Description\

    function transferOwnership(address newOwner) public override onlyOwner {
        if (newOwner == address(0)) {
            revert OwnableInvalidOwner(address(0));
        }

        if (!isClaimed && newOwner != ethMultiVault.getAtomWarden()) {
            isClaimed = true;
        }

@>      _transferOwnership(newOwner);
    }

The above function used in AtomWalletcontract is overrided from openzeppelin's OwnableUpgradeable.sol contract which is inherited by AtomWallet.sol contract to manage owner related functionalities. This is used to transfer ownership to users by wallet owner and this is not safe as the process is 1-step which is risky due to a possible human error and such an error is unrecoverable.

For example, an incorrect address, for which the private key is not known, could be passed accidentally and once the incorrect address becomes wallet owner then its impossible to recover.

Therefore, functions using onlyOwner modifier like execute(), executeBatch() and transferOwnership()will be locked and can not be used if the owner address is set incorrectly and in worst case the whole AtomWalletcontract will be of no use if such critical functions can not be accessed by real owner.

The funds deposited at entry point can not withdraw as withdrawDepositTo() would also need to be called by owner.

    function withdrawDepositTo(address payable withdrawAddress, uint256 amount) public {
        if (!(msg.sender == owner() || msg.sender == address(this))) {
            revert Errors.AtomWallet_OnlyOwner();
        }
        entryPoint().withdrawTo(withdrawAddress, amount);
    }

Impact\ The above discussed critical functions using the onlyOwner modifier in AtomWallet.sol will be locked and could not used if the issue happens.

Recommendations\

- import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
+ import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Ownable2StepUpgradeable.sol";

   . . . some code...

- contract AtomWallet is Initializable, BaseAccount, OwnableUpgradeable {
+ contract AtomWallet is Initializable, BaseAccount, Ownable2StepUpgradeable{
mihailo-maksa commented 1 week ago

Duplicate of issue #14.