Topmark - Incomplete Revert Implementation in the AccountV1 Contract

[sherlock-admin2]

Topmark

medium

Incomplete Revert Implementation in the AccountV1 Contract

Summary

Incomplete Revert Implementation during upgradeAccount(...) function call in the AccountV1 Contract

Vulnerability Detail

 function upgradeAccount(address newImplementation, address newRegistry, uint256 newVersion, bytes calldata data)
        external
        onlyFactory
        nonReentrant
        notDuringAuction
        updateActionTimestamp
    {
        // Cache old parameters.
        address oldImplementation = _getAddressSlot(IMPLEMENTATION_SLOT).value;
        address oldRegistry = registry;
        uint256 oldVersion = ACCOUNT_VERSION;

        // Store new parameters.
        _getAddressSlot(IMPLEMENTATION_SLOT).value = newImplementation;
        registry = newRegistry;

        // Prevent that Account is upgraded to a new version where the Numeraire can't be priced.
>>>        if (newRegistry != oldRegistry && !IRegistry(newRegistry).inRegistry(numeraire)) {
            revert AccountErrors.InvalidRegistry();
        }
...

The code above shows the implementation of the upgradeAccount(...) function in the AccountV1 contract, the problem is that the validation done in the code is incomplete as oldregistry can still be attached after the upgrade of account as the code does not strictly ensure newRegistry != oldRegistry, this validation would only stand when !IRegistry(newRegistry).inRegistry(numeraire) holds true which should not be so, the code should revert if newRegistry is not equal to oldRegistry

Impact

Incomplete Revert Implementation during upgradeAccount(...) function call in the AccountV1 Contract as code would not revert when newRegistry is not actually different from old Registry

Code Snippet

https://github.com/sherlock-audit/2023-12-arcadia/blob/main/accounts-v2/src/accounts/AccountV1.sol#L209

Tool used

Manual Review

Recommendation

As adjusted below the code should revert when newRegistry is not different from oldRegistry

 function upgradeAccount(address newImplementation, address newRegistry, uint256 newVersion, bytes calldata data)
        external
        onlyFactory
        nonReentrant
        notDuringAuction
        updateActionTimestamp
    {
        // Cache old parameters.
        address oldImplementation = _getAddressSlot(IMPLEMENTATION_SLOT).value;
        address oldRegistry = registry;
        uint256 oldVersion = ACCOUNT_VERSION;

        // Store new parameters.
        _getAddressSlot(IMPLEMENTATION_SLOT).value = newImplementation;
        registry = newRegistry;

        // Prevent that Account is upgraded to a new version where the Numeraire can't be priced.
        if (newRegistry != oldRegistry && !IRegistry(newRegistry).inRegistry(numeraire)) {
            revert AccountErrors.InvalidRegistry();
        }
+++  if (newRegistry != oldRegistry ) {
+++            revert AccountErrors.InvalidRegistry();
+++        }
...

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

takarez commented:

invalid

[nevillehuang]

Invalid, no issue here check is correct, asset must be present in registry to initiate an account upgrade