Topmark - Transfer of Ownership without Cool Down Validation would trigger Account Action that might be disadvantageous to New Owner

[sherlock-admin2]

Topmark

high

Transfer of Ownership without Cool Down Validation would trigger Account Action that might be disadvantageous to New Owner

Summary

Transfer of Ownership without Cool Down Validation would trigger Account Action that might be disadvantageous to New Owner

Vulnerability Detail

  /**
     * @notice Transfers ownership of the contract to a new Account.
     * @param newOwner The new owner of the Account.
     * @dev Can only be called by the current owner via the Factory.
     * A transfer of ownership of the Account is triggered by a transfer
     * of ownership of the accompanying ERC721 Account NFT, issued by the Factory.
     * Owner of Account NFT = owner of Account
     * @dev Function uses a cool-down period during which ownership cannot be transferred.
>>>     * Cool-down period is triggered after any account action, that might be disadvantageous for a new Owner.
     * This prevents the old Owner from frontrunning a transferFrom().
     */
    function transferOwnership(address newOwner) external onlyFactory notDuringAuction {
>>>        if (block.timestamp <= lastActionTimestamp + COOL_DOWN_PERIOD) revert AccountErrors.CoolDownPeriodNotPassed();

        // The Factory will check that the new owner is not address(0).
        owner = newOwner;
    }

The code above shows how transfer of Ownership is handled in the AccountV1 contract, point of interest is the time cool down validation that is ensured and confirmed before transfer of ownership is done, it can also be noted in the comment description as pointed out how this cool down validation is of importance as it helps to prevent problems for the new owner. However the auctionBoughtIn function below in the same contract shows how ownership transfer can also be done through another route, the problem is that cooldown was not put into consideration in this other route, which opens the problem that cool down time was suppose to solve in the first place.

 function auctionBoughtIn(address recipient) external onlyLiquidator nonReentrant {
        _transferOwnership(recipient);
    }
...
>>>    function _transferOwnership(address newOwner) internal {
        // The Factory will check that the new owner is not address(0).
        owner = newOwner;
        IFactory(FACTORY).safeTransferAccount(newOwner);
    }

Impact

Transfer of Ownership without Cool Down Validation would trigger Account Action that might be disadvantageous to New Owner

Code Snippet

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

Tool used

Manual Review

Recommendation

The protocol should treat the use of Cool down at utmost importance in every possible route of account ownership transfer, as adjusted in the code below, a necessary validation to ensure cooldown is confirmed should be added to the _transferOwnership(...) function just as it is present in the transferOwnership(...) function.

 function auctionBoughtIn(address recipient) external onlyLiquidator nonReentrant {
        _transferOwnership(recipient);
    }
...
    function _transferOwnership(address newOwner) internal {
+++  if (block.timestamp <= lastActionTimestamp + COOL_DOWN_PERIOD) revert AccountErrors.CoolDownPeriodNotPassed();
        // The Factory will check that the new owner is not address(0).
        owner = newOwner;
        IFactory(FACTORY).safeTransferAccount(newOwner);
    }

[sherlock-admin2]

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

takarez commented:

valid: the attack is still possible; medium(3)

takarez commented:

valid: cool down period is not implemented in that function; medium(8)

[nevillehuang]

Invalid, in auctionBoughtIn() the entire account is already being subjected to an auction that did not end within the cutOffTime, wherein old owner do not have the ability to front-run a transferFrom() given this indicates a manual liquidation.