Skip to content
This repository has been archived by the owner on Nov 19, 2023. It is now read-only.

xiaoming90 - Secondary debt dust balances are not truncated #210

Open
sherlock-admin opened this issue May 15, 2023 · 4 comments
Open

xiaoming90 - Secondary debt dust balances are not truncated #210

sherlock-admin opened this issue May 15, 2023 · 4 comments
Labels
Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@sherlock-admin
Copy link
Contributor

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.

@github-actions github-actions bot added the Medium A valid Medium severity issue label May 19, 2023
@jeffywu jeffywu added the Sponsor Confirmed The sponsor acknowledged this issue is valid label May 29, 2023
@jeffywu
Copy link

jeffywu commented May 29, 2023

Valid, medium severity looks good

@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label May 29, 2023
@jeffywu
Copy link

jeffywu commented Jul 11, 2023

Fixed in: notional-finance/contracts-v2#137

@jeffywu jeffywu added the Will Fix The sponsor confirmed this issue will be fixed label Jul 11, 2023
@xiaoming9090
Copy link
Collaborator

@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 notional-finance/contracts-v2#137

@jacksanford1
Copy link

Sherlock note: Classifying this as fixed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

4 participants