Coinspect BOLD-03: Adversaries can deflate batch debt to harm new troves upon redistribution

[bingen]

Trove owners inside a batch are able to manipulate the value of batchDebtShares by repaying small amounts of debt. As a consequence, the batch's debt is reduced leaving the amount of shares constant. This misalignment then impacts when new troves are opened, granting them more debt shares.
Shares of a batch are updated inside _updateBatchShares when a trove of the batch is touched. Upon debt decrease, shares are updated as follows:

// Subtract debt
batchDebtSharesDelta = currentBatchDebtShares * debtDecrease / _batchDebt;
Troves[_troveId].batchDebtShares -= batchDebtSharesDelta;
batches[_batchAddress].debt = _batchDebt - debtDecrease;
batches[_batchAddress].totalDebtShares = currentBatchDebtShares - batchDebtSharesDelta;

Making small debt decreases to a trove, leads to batchDebtSharesDelta being zero for non-zero debt decreases (rounds down). Then, when a new trove is opened in the same batch more debt shares are assigned to that trove. This
happens because the batchDebt decreased but the batch shares remained constant after the manipulation:

    // To avoid rebasing issues, let’s make sure the ratio debt / shares is not too high
    _requireBelowMaxSharesRatio(currentBatchDebtShares, _batchDebt,
    _checkBatchSharesRatio);
    batchDebtSharesDelta = currentBatchDebtShares * debtIncrease / _batchDebt;
}
Troves[_troveId].batchDebtShares += batchDebtSharesDelta;
batches[_batchAddress].debt = _batchDebt + debtIncrease;
batches[_batchAddress].totalDebtShares = currentBatchDebtShares + batchDebtSharesDelta;

This issue is considered to have low likelihood since the attacker requires to spend resources to considerably imbalance the share calculation. However, the likelihood increases for networks that are uncongested or have low gas fees. The impact is assessed to be medium as the victim will be entitled with more debt than they should upon debt redistribution.

[bingen]

See test with PoC here: #548

[bingen]

While it’s true that new troves may have more shares, that wouldn’t translate into more debt, so I don’t think it’s an issue, unless maybe if the ratio shares/debt is crazily high.
Besides:

• The system tends naturally towards the opposite: interest and fees provokes that debt increases, but shares not. So unless the batch is quite new, it would be harder to reverse that ratio.
• As mentioned in the issue, all the imbalance between shares and debt has to correspond to an amount of debt repaid by the attacker but not subtracted from the trove. So it’s not profitable.

[cupOJoseph]

I feel like this could be easily fixed with a

require(debtDecrease > 100e9 || debtDecrease == 0, "batch debt changes must be more than 100 gwei when decreasing debt or your new debt must be 0.");

Right after debtIncrease and debtDecrease have been set to the nominal values. ~line 1801

Fixed function: https://gist.github.com/cupOJoseph/a7c4b73a479f8616734f0087b5d8702d

This would prevent any net changes with small decreases, for example increase by 1eth, decrease by 1.00000000001eth
That could be good or bad

[GalloDaSballo]

https://gist.github.com/cupOJoseph/a7c4b73a479f8616734f0087b5d8702d

Heads up, this change can cause DOS to Redemptions due to the fact that redemptions can lead to changes of those amounts to the next trove in the batch