Arrays in cases where length = 0 leads to unexpected behaviour; should be checking that array length is not empty
It is not uncommon for front ends to send in default values by default or user making an error and can result in empty intentionally or unintentionally arrays being passed into functions. If such empty arrays where not carefully considered as inputs in the function logic, they can lead to unexpected behaviour. Consider the examples below
function liquidateSecondary(
address asset,
MTypes.BatchMC[] memory batches,
uint88 liquidateAmount,
bool isWallet
) external onlyValidAsset(asset) isNotFrozen(asset) nonReentrant {
STypes.AssetUser storage AssetUser = s.assetUser[asset][msg.sender];
MTypes.MarginCallSecondary memory m;
uint256 minimumCR = LibAsset.minimumCR(asset);
uint256 oraclePrice = LibOracle.getSavedOrSpotOraclePrice(asset);
uint256 secondaryLiquidationCR = LibAsset.secondaryLiquidationCR(asset);
uint88 liquidatorCollateral;
uint88 liquidateAmountLeft = liquidateAmount;
for (uint256 i; i < batches.length;) {
m = _setMarginCallStruct(
asset, batches[i].shorter, batches[i].shortId, minimumCR, oraclePrice
);
unchecked {
++i;
}
if (
m.shorter == msg.sender || m.cRatio > secondaryLiquidationCR
|| m.short.status == SR.Cancelled
|| m.short.id >= s.assetUser[asset][m.shorter].shortRecordId
|| m.short.id < Constants.SHORT_STARTING_ID
|| (m.shorter != address(this) && liquidateAmountLeft < m.short.ercDebt)
) {
continue;
}
bool partialTappLiquidation;
if (m.shorter == address(this)) {
partialTappLiquidation = liquidateAmountLeft < m.short.ercDebt;
if (partialTappLiquidation) {
m.short.ercDebt = liquidateAmountLeft;
}
}
if (isWallet) {
IAsset tokenContract = IAsset(asset);
uint256 walletBalance = tokenContract.balanceOf(msg.sender);
if (walletBalance < m.short.ercDebt) continue;
tokenContract.burnFrom(msg.sender, m.short.ercDebt);
assert(tokenContract.balanceOf(msg.sender) < walletBalance);
} else {
if (AssetUser.ercEscrowed < m.short.ercDebt) {
continue;
}
AssetUser.ercEscrowed -= m.short.ercDebt;
}
if (partialTappLiquidation) {
_secondaryLiquidationHelperPartialTapp(m);
} else {
_secondaryLiquidationHelper(m);
}
liquidatorCollateral += m.liquidatorCollateral;
liquidateAmountLeft -= m.short.ercDebt;
if (liquidateAmountLeft == 0) break;
}
if (liquidateAmount == liquidateAmountLeft) {
revert Errors.MarginCallSecondaryNoValidShorts();
}
s.asset[asset].ercDebt -= liquidateAmount - liquidateAmountLeft;
s.vaultUser[m.vault][msg.sender].ethEscrowed += liquidatorCollateral;
emit Events.LiquidateSecondary(asset, batches, msg.sender, isWallet);
}
Above function if batches.length = 0 will skip the for loop part and reach the // Update finalized state changes; although this is not a problem as updates are on zero values. A problem is that an event is emitted 'emit Events.LiquidateSecondary(asset, batches, msg.sender, isWallet);' which gives offchain event monitoring, tooling, reporting, tracking and users and or front ends impression secondary liquidations are happening when nothing is happening as no batches supplied etc. It is ideal if batches.length = 0 to revert function such that no event is emitted.
function distributeYield(address[] calldata assets) external nonReentrant {
uint256 length = assets.length;
uint256 vault = s.asset[assets[0]].vault;
(uint88 yield, uint256 dittoYieldShares) = _distributeYield(assets[0]);
for (uint256 i = 1; i < length;) {
if (s.asset[assets[i]].vault != vault) revert Errors.DifferentVaults();
(uint88 amtYield, uint256 amtDittoYieldShares) = _distributeYield(assets[i]);
yield += amtYield;
dittoYieldShares += amtDittoYieldShares;
unchecked {
++i;
}
}
_claimYield(vault, yield, dittoYieldShares);
emit Events.DistributeYield(vault, msg.sender, yield, dittoYieldShares);
}
As in previous example will result in emission event DistributeYield(vault, msg.sender, yield, dittoYieldShares) even if yield not distributed as address[] calldata assets = [] empty array.
In above examples it leads to misleading events e.g make it seem as if a lot of secondary liquidations are taking place in the protocol this may misinform users, monitoring tools, offchain reporting, fronts ends, security checks etc that rely on these events
It is recommended to ensure that such functions that may need to ensure arrays are not empty be checked early in the code to ensure function reverts
