`account.holdsToken` is never set

[code423n4]

Email address

mail@cmichel.io

Handle

@cmichelio

Eth address

0x6823636c2462cfdcD8d33fE53fBCD0EdbE2752ad

Vulnerability details

The addHolding function does not update the account.holdsToken map.

function addHolding(
    CrossMarginAccount storage account,
    address token,
    uint256 depositAmount
) internal {
    if (!hasHoldingToken(account, token)) {
        // SHOULD SET account.holdsToken here
        account.holdingTokens.push(token);
    }

    account.holdings[token] += depositAmount;
}

This leads to a critical vulnerability where deposits of the same token keep being pushed to the account.holdingTokens array but the sum is correctly updated in account.holdings[token].

However, because of the duplicate token in the holdingTokens array the same token is counted several times in the getHoldingAmounts function:

function getHoldingAmounts(address trader)
    external
    view
    override
    returns (
        address[] memory holdingTokens,
        uint256[] memory holdingAmounts
    )
{
    CrossMarginAccount storage account = marginAccounts[trader];
    holdingTokens = account.holdingTokens;

    holdingAmounts = new uint256[](account.holdingTokens.length);
    for (uint256 idx = 0; holdingTokens.length > idx; idx++) {
        address tokenAddress = holdingTokens[idx];
        // RETURNS SUM OF THE BALANCE FOR EACH TOKEN ENTRY
        holdingAmounts[idx] = account.holdings[tokenAddress];
    }
}

The MarginRouter.crossCloseAccount function uses these wrong amounts to withdraw all tokens:

function crossCloseAccount() external {
    (address[] memory holdingTokens, uint256[] memory holdingAmounts) =
        IMarginTrading(marginTrading()).getHoldingAmounts(msg.sender);

    // requires all debts paid off
    IMarginTrading(marginTrading()).registerLiquidation(msg.sender);

    for (uint256 i; holdingTokens.length > i; i++) {
        Fund(fund()).withdraw(
            holdingTokens[i],
            msg.sender,
            holdingAmounts[i]
        );
    }
}

Impact

An attacker can just deposit the same token X times which increases their balance by X times the actual value. This inflated balance can then be withdrawn to steal all tokens.

Recommended mitigation steps

Correctly set the account.holdsToken map in addHolding.