Underlying token percents should add up to 100% / exposure.sortVaultsByDelta tracking does not work

[code423n4]

Handle

cmichel

Vulnerability details

Vulnerability Details

The Insurance.setUnderlyingTokenPercent function sets the target allocations per vault which is used throughout the code ("delta").
It does not validate that the sum of the underlyingTokensPercents is 100%, it could be higher or lower.

If the sum is less than 100% it leads to severe issues in parts of the code, for example, it's used in getVaultDeltaForDeposit to get the sorted vaultIndexes through this call exposure.sortVaultsByDelta.

However, exposure.sortVaultsByDelta has a bug with tracking min and max indexes if they don't add up to 100%, because the else branch of minDelta must never be taken.

Example bug:
Assume: unifiedTotalAssets = 100k, unifiedAssets = [40k, 30k, 30k], targetPercents = [30%, 30%, 30%].

Then for i = 0:
delta = 40k - 100k * 30% = 10k > 0 => sets maxDelta = 10k, maxIndex = 0
Then for i = 1:
delta = 30k - 100k * 30% = 0 > 0 is false but else branch with 0 < 0 is false as well => nothing is set
Then for i = 2:
delta = 30k - 100k * 30% = 0 > 0 is false but else branch with 0 < 0 is false as well => nothing is set

Resulting in both maxIndex = 0, minIndex = 0 and thus vaultIndexes = [0, 3, 0] depositing into the first vault twice and completely skipping the others.

Recommended Mitigation Steps

Validate that it adds up to 100%.
Alternatively, fix the minDelta and maxDelta starting values (0 is bad) and computation.

[kitty-the-kat]

low risk:
Insurance.setUnderlyingTokenPercent is set by governance and will be behind a timelock
similar issue in #11, which is set to low risk, which I agree with

[ghoul-sol]

This requires human error to be an issue. While I agree that best practices recommend data validation, I'd imagine that governance TX would be checked 10 times before sending. I'm agreeing with sponsor, this is low risk.

[ghoul-sol]

Duplicate of #11