Topmark - Wrong Implementation in the openMarginAccount Function

[sherlock-admin2]

Topmark

medium

Wrong Implementation in the openMarginAccount Function

Summary

Wrong Implementation in the openMarginAccount Function of the AccountV1 contract as New MarginAccount is Opened before closing new One instead of the other way round

Vulnerability Detail

 function openMarginAccount(address newCreditor)
        external
        onlyOwner
        nonReentrant
        notDuringAuction
        updateActionTimestamp
    {
        (address[] memory assetAddresses, uint256[] memory assetIds, uint256[] memory assetAmounts) =
            generateAssetData();

        // Cache old Creditor.
        address oldCreditor = creditor;
        if (oldCreditor == newCreditor) revert AccountErrors.CreditorAlreadySet();

        // Remove the exposures of the Account for the old Creditor.
        if (oldCreditor != address(0)) {
            IRegistry(registry).batchProcessWithdrawal(oldCreditor, assetAddresses, assetIds, assetAmounts);
        }

        // Check if all assets in the Account are allowed by the new Creditor
        // and add the exposure of the account for the new Creditor.
        IRegistry(registry).batchProcessDeposit(newCreditor, assetAddresses, assetIds, assetAmounts);

        // Open margin account for the new Creditor.
>>>        _openMarginAccount(newCreditor);

        // A margin account can only be opened for one Creditor at a time.
        // If set, close the margin account for the old Creditor.
        if (oldCreditor != address(0)) {
            // closeMarginAccount() checks if there is still an open position (open liabilities) of the Account for the old Creditor.
            // If so, the function reverts.
 >>>           ICreditor(oldCreditor).closeMarginAccount(address(this));
        }
    }

The code provided above shows how openMarginAccount(...) function is implemented, as pointed out in the pointer above the Protocol Opned a new MarginAccount before closing the old MarginAccount, this could cause problem for the protocol due to clashing of variable state values between the old data and the new data, before A new Margin would be opened the old one should be closed first to avoid counter interactions

Impact

Wrong Implementation in the openMarginAccount Function of the AccountV1 contract as New MarginAccount is Opened before closing new One instead of the other way round

Code Snippet

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

Tool used

Manual Review

Recommendation

Protocol should make necessary adjustments to ensure a new openMarginAccount is only opened after the old one has been closed

[sherlock-admin2]

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

takarez commented:

invalid

[nevillehuang]

Invalid, no issue here, oldCreditor is first cached with the original creditor before replacement as seen here