sherlock-audit / 2023-03-notional-judging

12 stars 6 forks source link

xiaoming90 - Secondary debt dust balances are not truncated #210

Open sherlock-admin opened 1 year ago

sherlock-admin commented 1 year ago

xiaoming90

medium

Secondary debt dust balances are not truncated

Summary

Dust balances in primary debt are truncated toward zero. However, this truncation was not performed against secondary debts.

Vulnerability Detail

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L231

File: VaultAccount.sol
212:     function updateAccountDebt(
..SNIP..
230:         // Truncate dust balances towards zero
231:         if (0 < vaultState.totalDebtUnderlying && vaultState.totalDebtUnderlying < 10) vaultState.totalDebtUnderlying = 0;
..SNIP..
233:     }

vaultState.totalDebtUnderlying is primarily used to track the total debt of primary currency. Within the updateAccountDebt function, any dust balance in the vaultState.totalDebtUnderlying is truncated towards zero at the end of the function as shown above.

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultSecondaryBorrow.sol#L304

File: VaultSecondaryBorrow.sol
304:     function _updateTotalSecondaryDebt(
305:         VaultConfig memory vaultConfig,
306:         address account,
307:         uint16 currencyId,
308:         uint256 maturity,
309:         int256 netUnderlyingDebt,
310:         PrimeRate memory pr
311:     ) private {
312:         VaultStateStorage storage balance = LibStorage.getVaultSecondaryBorrow()
313:             [vaultConfig.vault][maturity][currencyId];
314:         int256 totalDebtUnderlying = VaultStateLib.readDebtStorageToUnderlying(pr, maturity, balance.totalDebt);
315:         
316:         // Set the new debt underlying to storage
317:         totalDebtUnderlying = totalDebtUnderlying.add(netUnderlyingDebt);
318:         VaultStateLib.setTotalDebtStorage(
319:             balance, pr, vaultConfig, currencyId, maturity, totalDebtUnderlying, false // not settled
320:         );

However, this approach was not consistently applied when handling dust balance in secondary debt within the _updateTotalSecondaryDebt function. Within the _updateTotalSecondaryDebt function, the dust balance in secondary debts is not truncated.

Impact

The inconsistency in handling dust balances in primary and secondary debt could potentially lead to discrepancies in debt accounting within the protocol, accumulation of dust, and result in unforeseen consequences.

Code Snippet

https://github.com/sherlock-audit/2023-03-notional-0xleastwood/blob/main/contracts-v2/contracts/internal/vaults/VaultSecondaryBorrow.sol#L304

Tool used

Manual Review

Recommendation

Consider truncating dust balance in secondary debt within the _updateTotalSecondaryDebt function similar to what has been done for primary debt.

jeffywu commented 1 year ago

Valid, medium severity looks good

jeffywu commented 1 year ago

Fixed in: https://github.com/notional-finance/contracts-v2/pull/137

xiaoming9090 commented 1 year ago

@0xleastwood + @xiaoming9090 : Understood from the team that the truncation of dust is no longer necessary. Thus, they have been removed from the codebase. Update made in PR https://github.com/notional-finance/contracts-v2/pull/137

jacksanford1 commented 1 year ago

Sherlock note: Classifying this as fixed.