DittoETH

Summary

cannot checked the value in the for loop ._onlyValidShortRecord this function the fist id cannot checked.

Vulnerability Details

function combineShorts(address asset, uint8[] memory ids)
external
isNotFrozen(asset)
nonReentrant
onlyValidShortRecord(asset, msg.sender, ids[0])
{
if (ids.length < 2) revert Errors.InsufficientNumberOfShorts();
// First short in the array
STypes.ShortRecord storage firstShort = s.shortRecords[asset][msg.sender][ids[0]];
// @dev Load initial short elements in struct to avoid stack too deep
MTypes.CombineShorts memory c;
c.shortFlagExists = firstShort.flaggerId != 0;
c.shortUpdatedAt = firstShort.updatedAt;

    address _asset = asset;
    uint88 collateral;
    uint88 ercDebt;
    uint256 yield;
    uint256 ercDebtSocialized;
    for (uint256 i = ids.length - 1; i > 0; i--) {
        //@audit check that i>0 is not correct.
        uint8 _id = ids[i];
        _onlyValidShortRecord(_asset, msg.sender, _id);
        STypes.ShortRecord storage currentShort =
            s.shortRecords[_asset][msg.sender][_id];
        // See if there is at least one flagged short
        if (!c.shortFlagExists) {
            if (currentShort.flaggerId != 0) {
                c.shortFlagExists = true;
            }
        }

        //@dev Take latest time when combining shorts (prevent flash loan)
        if (currentShort.updatedAt > c.shortUpdatedAt) {
            c.shortUpdatedAt = currentShort.updatedAt;
        }

        {
            uint88 currentShortCollateral = currentShort.collateral;
            uint88 currentShortErcDebt = currentShort.ercDebt;
            collateral += currentShortCollateral;
            ercDebt += currentShortErcDebt;
            yield += currentShortCollateral.mul(currentShort.zethYieldRate);
            ercDebtSocialized += currentShortErcDebt.mul(currentShort.ercDebtRate);
        }

        if (currentShort.tokenId != 0) {
            //@dev First short needs to have NFT so there isn't a need to burn and re-mint
            if (firstShort.tokenId == 0) {
                revert Errors.FirstShortMustBeNFT();
            }

            LibShortRecord.burnNFT(currentShort.tokenId);
        }

        // Cancel this short and combine with short in ids[0]
        LibShortRecord.deleteShortRecord(_asset, msg.sender, _id);
    }

    // Merge all short records into the short at position id[0]
    firstShort.merge(ercDebt, ercDebtSocialized, collateral, yield, c.shortUpdatedAt);

    // If at least one short was flagged, ensure resulting c-ratio > primaryLiquidationCR
    if (c.shortFlagExists) {
        if (
            firstShort.getCollateralRatioSpotPrice(
                LibOracle.getSavedOrSpotOraclePrice(_asset)
            ) < LibAsset.primaryLiquidationCR(_asset)
        ) revert Errors.InsufficientCollateral();
        // Resulting combined short has sufficient c-ratio to remove flag
        firstShort.resetFlag();
    }
    emit Events.CombineShorts(asset, msg.sender, ids);
}

}
in the above one in for loop i>0 checked . so that it cannot checked the first value.

Impact

cannot checked the first value in the for loop so it con't checked the _onlyValidShortRecord(_asset, msg.sender, _id); it leads to that _id record not valid.

Tools Used

manual

Recommendations

    for (uint256 i = ids.length - 1; i >= 0; i--) 

this can check the first value also.