Topmark - Old Owner can take Advantage of updateActionTimestamp Modifier Absence to Front RunAccountV1 contract Against New Owner

[sherlock-admin]

Topmark

medium

Old Owner can take Advantage of updateActionTimestamp Modifier Absence to Front RunAccountV1 contract Against New Owner

Summary

Old Owner can take Advantage of updateActionTimestamp Modifier Absence to front Run closeMarginAccount(...) function call againt Against New Owner

Vulnerability Detail

   /**
     * @dev Starts the cool-down period during which ownership cannot be transferred.
     * This prevents the old Owner from frontrunning a transferFrom().
     */
    modifier updateActionTimestamp() {
        lastActionTimestamp = uint32(block.timestamp);
        _;
    }

The modifier provided above shows how updateActionTimestamp is handles, it can be noted in the comment description that oldowner can take advantage of New Owner though Front Running, which is why this modifier is present in many part of the contract by different function calls however the problem is that this modifier is absent in the closeMarginAccount(...) function call which can be taken advantage of by a bad actor in other to create discrepancies for the new owner immediately after transfer of ownership

 function closeMarginAccount() external onlyOwner nonReentrant notDuringAuction {
        // Cache creditor.
        address creditor_ = creditor;
        if (creditor_ == address(0)) revert AccountErrors.CreditorNotSet();

        creditor = address(0);
        liquidator = address(0);
        minimumMargin = 0;

        // Remove the exposures of the Account for the old Creditor.
        (address[] memory assetAddresses, uint256[] memory assetIds, uint256[] memory assetAmounts) =
            generateAssetData();
        IRegistry(registry).batchProcessWithdrawal(creditor_, assetAddresses, assetIds, assetAmounts);

        // closeMarginAccount() checks if there is still an open position (open liabilities) for the Account.
        // If so, the function reverts.
        ICreditor(creditor_).closeMarginAccount(address(this));

        emit MarginAccountChanged(address(0), address(0));
    }

  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;
    }

Impact

Old Owner can Manipulate contract through Frontrunning

Code Snippet

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

Tool used

Manual Review

Recommendation

The modifer should be added to the closeMarginAccount() function as provided below

---  function closeMarginAccount() external onlyOwner nonReentrant notDuringAuction {
+++  function closeMarginAccount() external onlyOwner nonReentrant notDuringAuction updateActionTimestamp {
        // Cache creditor.
        address creditor_ = creditor;
        if (creditor_ == address(0)) revert AccountErrors.CreditorNotSet();

        creditor = address(0);
        liquidator = address(0);
        minimumMargin = 0;

        // Remove the exposures of the Account for the old Creditor.
        (address[] memory assetAddresses, uint256[] memory assetIds, uint256[] memory assetAmounts) =
            generateAssetData();
        IRegistry(registry).batchProcessWithdrawal(creditor_, assetAddresses, assetIds, assetAmounts);

        // closeMarginAccount() checks if there is still an open position (open liabilities) for the Account.
        // If so, the function reverts.
        ICreditor(creditor_).closeMarginAccount(address(this));

        emit MarginAccountChanged(address(0), address(0));
    }

Duplicate of #1

[sherlock-admin2]

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

takarez commented:

invalid

[nevillehuang]

Invalid, agree with sponsors comments:

Closing a creditor should be seen as reducing risk (no more flash actions, no more getting debt against the account).