# Numa Vault V2 Review // 24-07-2024

### Scope
https://github.com/NumaMoney/Numa/tree/VaultRefacto
Numa/contracts/NumaProtocol
Numa/contracts/lending
Numa/contracts/nuAssets
Numa/contracts/libraries

### Commit hash and branch
[1a760477e74762619899e21e5ca937c57d4d0b63](https://github.com/NumaMoney/Numa/commit/1a760477e74762619899e21e5ca937c57d4d0b63)

## [H-1] Users can mint free NUMA if certain conditions are met by minting and burning the nuAsset immediately

### Summary
When some conditions are met minting and burning the nuAsset will result on NUMA profit although the Uniswap and vault price of NUMA is same.

### Vulnerability Details
Assume:
- 1000 NUMA circulating supply
- 100 ETH balance in the vault
- 80 ETH balance in synthetics

Alice has 100 NUMA and wants to mint nuUSD. Alice calls the `mintAssetFromNumaInput` function, which triggers the internal `getNbOfNuAssetFromNuma` function.

First, we have a burn fee of 5%, which means 5 NUMA will be burnt, and 95 NUMA worth of synthetics will be minted.

1. **Calculating the value of 100 NUMA:**

1000 (100 - 80) = 2 ETH

- Adding the buy fee:

2 * 1.05 = 2.1
So, `EthPerNumaVault` is 2.10 ETH.

However, when we pass the NUMA amount to be priced to the oracle contract, we pass the `_numaAmount - amountToBurn` value, which is 95 NUMA.

In the NumaOracle contract, if the Uniswap pool amount calculation is lower than the vault calculations, it will be used. Since we are pricing 95 NUMA and the vault's amount is calculated with 100 NUMA, the Uniswap pool amount will almost always be cheaper due to the mismatch in amounts. Assuming the price of NUMA is exactly equal on Uniswap and the Vault:

- Price of 95 NUMA:
1.95 ETH (we will use 1.9 for simplicity)

Then, at the end of the function, 100 NUMA will be burnt, but only 1.90 ETH worth of nuUSD is minted to Alice.

New balances:
- Circulating NUMA: 900
- Vault balance: 100 ETH
- Synth balance: 81.9 ETH (rounding 1.95 to 1.9 for simplicity)

Now, immediately after this transaction, Alice burns the 1.9 ETH worth of nuUSD by calling the `burnAssetInputToNuma`. `numaPerEthVault` will be calculated as:

1.9 * 900 / (100 - 81.49) = 94.47
Then, assuming a 10% sell fee:

94.47 * 1000 / 900 = 104.96

Since we will burn the 1.9 ETH worth of synthetics, if the NUMA amount to be minted after the burn and scale fee is higher than 100 NUMA, the user will always profit!

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaPrinter.sol#L434-L458

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaPrinter.sol#L231-L249

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaOracle.sol#L430-L433

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaPrinter.sol#L499-L527

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaPrinter.sol#L359-L389
### Impact
Free money by minting and burning the NUMA
### Recommendation
Few things here:
1- `_numaAmount - amountToBurn` or the `_numaAmount` should be used for both vaults amount calculation and the uniswaps
2- Buy and sell fees of vault, burn fees and scaling can create the above scenario even though you fix the "1"
3- Use the scaling when minting the nuAssets as well or don't use it when burning the nuAsset
Q - This is more of a question, if you have an apple and you want to know the price of the apple in terms of $ but apple can be sold to $2 and can be bought $1. What's the apple's price? $1 or $2?
- [x] Fixed
- [ ] Will not fix
- [x] In discussions

## [H-2] When minting nuAssets the fee calculation is done incorrectly

### Summary

When minting nuAssets the amount of NUMA in ETH is calculated by considering the buy fee. However, inclusion of the buy fee is not properly done.

### Vulnerability Details
Assume there are:
Circulating NUMA: 1000
Vault ETH balance: 100
Synths ETH balance: 80
and also, for simplicity, let's assume there are no scaling.

Alice has 100 NUMA and wants to mint 100 NUMA worth of nuUSD. Alice calls the `mintAssetFromNumaInput` function in the NumaPrinter contract which the internal `getNbOfNuAssetFromNuma` function is called to calculate the corresponding ETH amount of given NUMA amount including the buy fee.

first, this line will be executed:
`uint256 EthPerNumaVault = vaultManager.GetNumaPriceEth(_numaAmount);`

`VaultManager::GetNumaPriceEth()` (change this function name to camel case) function does not include the fee and simply follows the formula to calculate given NUMA amount in terms of ETH.

By our formula the return value of this will be:

$100 * (100 - 80) / 1000 = 2 ETH$

Then the buy fees are added on top of it by this line:
`EthPerNumaVault = EthPerNumaVault + (EthPerNumaVault * (1000-vaultManager.getBuyFee())) /1000;`

Assuming a buy fee of 5%, the new `EthPerNumaVault` (change this variable to camel case)

$2 + (2 * (1000 - 950) / 1000) = 2.1 ETH$

However, this fee inclusion is incorrect. Let's see how much 2.1 ETH would buy us in NUMA if we were to call `buy()` function in NumaVault.

Applying the same formula:

$2.1 * 1000 / (100 - 80) = 105 NUMA$

Then, inside the buy function again in NumaVault
`_numaOut = (numaAmount * fee) / BASE_1000;`

will be used. "fee" is %5, which is 950 in solidity code. Hence, this amount will be:
$105 * 950 / 1000 = 99.75 NUMA$

**Note that it is not 100 NUMA as we calculated when minting the nuUSD.**

The reason for this is how the fee is integrated.

Let's do some math to find out the correct calculation.

A: Eth amount user provides
TS: Total circulating NUMA supply
V: Vaults ETH balances
S: Synthetics ETH balances
F: Buy fee (complementary version, 5% -> 95%)

Numa amount without the fee (NA):
$$
A * TS / (V - S) = NA
$$

Then, after the buy fee the actual NUMA minted (ANA):

$$
NA - NA * fee = ANA
$$

When users call the NumaPrinter's `mintAssetFromNumaInput` function, the input `_numaAmount` is the "ANA" and we need to solve for the "A". Which is the ETH amount needed considering the fees.

Now, let's arrange our formula to solve for A:

$$
A = ANA * (V - S) / TS * (1-F)
$$

Now, let's do this formula instead of the current formula to see if it works:

ANA: 100 NUMA
TS: 1000 NUMA
V: 100 ETH
S: 80 ETH
F: 50 (5%)

$100 * (100 - 80) / 1000 * (1 - 0.05) = 2.105263158 ETH$

Now, let's use this amount and do a `buy()` to see if we will indeed end up with 100 NUMA:

$2.105263158 * 1000 / (100 - 80) = 105.2631579 NUMA$

Then, lets add the 5% buy fee:

$105.2631579 - 105.2631579 * 5 / 100 = 100 NUMA$

Hence, in order to mint exactly 100 NUMA 2.105263158 ETH is needed!

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/VaultManager.sol#L608-L655

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L485-L565

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaPrinter.sol#L434-L458
### Impact
Incorrect nuAsset minting to user
### Recommendation
Use the recommended formula above
- [x] Fixed
- [ ] Will not fix
- [ ] In discussions

## [H-3] Burning nuAssets miscalculates the NUMA amount due to double scaling

### Summary
When the amount of NUMA minted is calculated scaling is done twice which results in an incorrect amount of NUMA minted to user

### Vulnerability Details

`NumaPrinter::getNbOfNumaFromAssetWithFee()` internal function first calls the `VaultManager::GetNumaPerEth()` function (change it to camel case) to calculate the given NUMA in terms of ETH. As we can observe in the `GetNumaPerEth()` function, the formula already uses the synth scaling.

Then, following the lines scaling is applied once again:
`_output = (_output*scaleOverride)/BASE_1000;`

Resulting in an incorrect amount of NUMA to be minted for the user

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/VaultManager.sol#L657-L710

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaPrinter.sol#L499-L527

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaPrinter.sol#L359-L389
### Impact
Incorrect amount of NUMA minted to user
### Recommendation
Check [H-4 Recommendation](https://hackmd.io/8KUmhZhLTJWFq7WDSPYfhw?view#Recommendation20)
- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions

## [H-4] Interest is not updated when minting and burning nuAssets

### Summary
Whenever the supply, total eth balance of vault or synthetics balance changes the interest needs to be updated prior. However, this is not done when mint/burn nuAssets.

### Vulnerability Details

Since the utilization rate will change and the borrow rate will change hence, the prior interest must need to be accrued for fair distribution of the interest.

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaPrinter.sol#L434-L556

### Impact
Unfair distribution of interest for the users and potential insolvency for the lending market
### Recommendation
Use a common pattern for both mint and burn of NUMA as follows:

    function nuAssetFunction(uint256 amount) external {
        // dev: first update interest!
        accrueInterestLending();
        vault.extractRewards();
        vaultManager.updateAll();

        (uint numaOrnuAssetAmount, uint burn) = getFunction(amount);

        // dev: if burn nuUSD, do: 
        // numa.mint(numaOrnuAssetAmount - burn, msg.sender);
        // nuAsset.burn(amount, msg.sender);

        // dev: if mint nuUSD, do:
        // numa.burn(amount, msg.sender)
        // nuAsset.mint(numaOrnuAssetAmount - burn, msg.sender);
    }

    function getFunction(uint ) external returns (uint256, uint256) {
        uint burn;
        uint output;
        // dev: if mint, do the burn calc here
        // burn = _amount * burn / 1000;

        // dev: call the vault manager, this already applies scaling!
        uint256 EthPerNumaVault = vaultManager.GetNumaPriceEth(_numaAmount);
        // dev: add the buy/sell fees
        EthPerNumaVault = ;

        output = oracle.xxx();
        // dev: if burn, do the burn calc here
        // burn = output * burn / 1000;

        return (output, burn);
    }

// Note, when minting nuAsset instead of applying the burn fee to NUMA and then calculate the synth maybe you can calculate the nuAsset without the burn fee and you can then use the fee to deduct the amount directly from nuAsset amount
i.e:
100 NUMA is 1000 nuUSD (no fee so far)
1000 - 1000*5/100 (apply the 5% burn fee here) = 950 nuUSD
mint user 950 nuUSD.

- [ ] Fixed
- [ ] Will not fix
- [x] In discussions

## [H-5] Wrong unit is used when burning nuUSD for NUMA

### Summary
When nuAsset is burnt for NUMA, vault managers function is called with an amount that is not denominated in ETH

### Vulnerability Details

`NumaPrinter::burnAssetInputToNuma()` functions first input is the amount of nuAsset that will be burnt which is in terms of the nuAsset.

Inside the internal `getNbOfNumaFromAssetWithFee` function first line is to calculate the given nuAsset to NUMA:
`uint256 numaPerEthVault = vaultManager.GetNumaPerEth(_nuAssetAmount);`

However, `VaultManager::GetNumaPerEth()` (change to camel case) input is expected to be in ETH terms and we are calling it with an input in terms of the nuAsset.

**Same applies in these functions as well:**
`getNbOfNumaNeededAndFee`
`getNbOfNumaFromAssetWithFeeView`
### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaPrinter.sol#L499-L519

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaPrinter.sol#L359-L389
### Impact
Incorrect minting of NUMA when burning nuAsset
### Recommendation
Before calling the get function first convert the nuAsset amount to ETH amount.
- [ ] Fixed
- [ ] Will not fix
- [x] In discussions

## [H-6] Swapping of nuAssets are incorrectly implemented

### Summary
NumaOracle `getNbOfNuAssetFromNuAsset` function passes the nuAsset amount however, the ETH amount is expected in nuAssetManager contract.

### Vulnerability Details
As we can observe the following lines in `NumaOracle::getNbOfNuAssetFromNuAsset()` function:

`uint256 nuAssetOutPerETHmulAmount = nuAManager.getTokenPerEth(_nuAssetOut,_nuAssetAmountIn);`

`getTokenPerEth` function of nuAssetManager contract is called with an input amount denominated in nuAsset. However, nuAssetManager contract expects the input variable in ETH denomination.

Another big problem in the same line is that the nuAssetManager getTokenPerEth function is called with the address of output nuAsset with the input amount of nuAssetAmountIn. This will end up calculating the given amount of nuAsset amountIn in nuAsset output address!
### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaOracle.sol#L293-L302
### Impact
Incorrect swap amounts between nuAssets
### Recommendation
1- Convert the nuAsset to ETH before calling the `getTokenPerEth`
2- Get the ETH amount of nuAssetIn `uint256 nuAssetOutPerETHmulAmount = nuAManager.getTokenPerEth(_nuAssetIn,_nuAssetAmountIn);`
3- Get the 1 nuAsset out in ETH terms
4- Do the swap in ETH terms
5- Mint the corresponding ETH value worth of nuAssetOut to user
- [ ] Fixed
- [ ] Will not fix
- [x] In discussions

## [H-7] Scaling is almost impossible due to `lastBlockTime` updates

### Summary
Scaling is only applies when the `deltaDebase` amount of time is passed. However, if the `deltaDebase` is not passed, the latest block time stamp is updated and an another `deltaDebase` time needed. Hence, de/rebasing can be impossible.

### Vulnerability Details
Both in `getSynthScaling` and `getSellFeeScaling` if the current block time stamp is lesser than the `lastBlockTime + deltaDebase` then the re/debasing will not be applicable. However, although the re/debasing is not possible due to this time restrict the `lastBlockTime` is updated to the current block timestamp which an another `deltaDebase` required to pass. In a scaled system with frequent interactions, re/debasing can be impossible.
### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/VaultManager.sol#L321-L420
### Impact
Re/debasing will not work as intended
### Recommendation
If the re/debasing is needed `if (currentCF < cf_severe)` or the `else` then don't update the latest block timestamp. Alternatively, you can apply the re/debasing by seconds passed instead of a big `deltaDebase` interval.
- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions

## [H-8] Leveraging can cause immediate liquidations or excessive NUMA mints

### Summary
Leveraging can cause big shifts in the system’s balances and scaling factors, leading to too much NUMA being minted. This can make a user’s debt spike immediately, putting them at risk of instant liquidation. The problem gets worse if there isn't enough liquidity, causing more strain on the vault and increasing liquidation risks for users.

### Vulnerability Details
Let's do an example:

- Total vault ETH balance: 100
- Total synth ETH balance: 80
- Total NUMA circulating supply: 1000
- Scaling value: 9/8 (1.125)
- 1 rETH: 1 ETH

Assume the user provides no `supplyAmount` and a `borrowAmount` of 500 NUMA.

The vault will mint 500 NUMA to cLST, which cLST will deposit into cNUMA and give the user 500 NUMA worth of cNUMA.

When the rETH needed is calculated for the 500 NUMA, this line will be responsible for it:
`uint borrowAmount = vault.getAmountIn(_borrowAmount);`

Assuming no buy fee:
$500 * (100 - 80 * 9/8) / 1000 = 5 rETH$

Hence, 5 rETH will be borrowed from the cLST token.

Let's assume there is no margin buffer and exactly 5 rETH is borrowed from the cLST token.

Now, assume 5 rETH is not available in the cLST token and actually, cLST has no rETH at all, which means the 5 rETH will be borrowed from the vault. When this happens, the total ETH held by the vault will be 95 ETH.

When the cLST token buys NUMA with 5 rETH, since the global CF is now even lower, the scaling will also change! This happens because although the current scaling amount was used when calculating the 5 rETH value, the contract's storage is not updated, hence the latest block is still the last interaction block! Assume the CF global is lower than the `cf_critical`, which means the scaling will now be even higher than its previous value (1.125), and let's assume it's 1.18 now.

Now, with 5 rETH, the amount of NUMA bought will be:

$5 * (1000) / (95 - 80 * 1.18) = 8333.3 NUMA$

500 NUMA will be burnt at the end of the function, and since 5 rETH bought the NUMA, the vault now has:
- 17,833.3 NUMA supply (500 burnt, excess sent to the user)
- 100 ETH vault balance (back at 100)
- 80 synth balance

The user has:
- 500 cNUMA worth of NUMA
- 7,833.3 NUMA in their wallet
- 5 rETH worth of debt

Now, let's see how much 5 rETH would be worth in the NUMA Vault:

Note that the scaling has just been updated, and it will be 1.18, although the CF global returned to its previous value after the block. However, scaling increased to its critical level!
$5 * (17833.3) / (100 - 80 * 1.18) = 15922.58929 NUMA$

So at the end of the day:

The user has:
- 500 cNUMA worth of NUMA
- 7,833.3 NUMA in their wallet
- and 5 rETH, which is worth 15,922.58929 NUMA in debt!!

Which means the user is liquidatable immediately after the leverage operation.

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/lending/CNumaToken.sol#L79-L126
### Impact
Immediate liquidations for leveragers
### Recommendation
...
- [ ] Fixed
- [ ] Will not fix
- [x] In discussions

## [H-9] Liquidations can be blocked by frontrunning the liquidator by repaying a tiny amount of debt

### Summary
If someone repays 1 wei worth of the asset just before the liquidation call of another person, the liquidation transaction would fail.

### Vulnerability Details

In Compound, to block the front-running repay to block liquidation issue, `repayAmount` can be passed in as `type(uint256).max`, allowing the code to interpret it as the maximum amount of debt the user has. This ensures that repaying just before the liquidation transaction will not work. However, since the liquidation call can't use `type(uint256).max` (due to the limitations of flashloans or the availability of funds from `msg.sender`), the liquidator must repay the exact amount of debt the borrower has to fully liquidate someone. If the borrower can repay just 1 wei just before the liquidate transaction, then the liquidator's liquidation function would revert.

### Links to Code
https://github.com/compound-finance/compound-protocol/blob/a3214f67b73310d547e00fc578e8355911c9d376/contracts/CToken.sol#L658-L659

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L979-L1317
### Impact
Liquidators can be griefed by the borrower. At the end, liquidators would not try to liquidate such account because of the failed transaction fees.
### Recommendation
Allow type(uint256.max) to be passed. Alternatively, you can have an additional input "bool repayAll" where if its called with true then reference the entire debt user has.
- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions
## [H-10] If a user has some debt repaid and holds some cTokens, redeeming those cTokens will be impossible

### Summary
If user calls the comptrollers `enterMarkets` function then the user can't redeem its supplied amount due to calculations in the `getHypotheticalAccountLiquidityIsolateInternal`

### Vulnerability Details
When a user enters the market by calling `enterMarkets` in Comptroller, `markets[cToken].accountMembership[account]` will be set to true.

If the user has no borrow balance and tries to redeem, the `redeemAllowedInternal` check will call the `getHypotheticalAccountLiquidityIsolateInternal` internal function. Inside this function, since the user does not have any "collateral" token (if the user has cNUMA, then it's cLST and vice versa), the user's total collateral will be calculated as "0". Consequently, the user will always have a shortfall because `0 - redeemAmount` will be negative. Thus, the user will not be able to redeem, and the funds will be stuck.

Calling `exitMarkets` will not help the user either, as it performs the same check.

**Textual PoC:**
Assume Alice has 100 cNUMA and a debt of 1 rETH.

In this scenario, collateral are 100 cNUMA and debt is 1 rETH and Alice entered both of the markets.

When Alice repays the 1 rETH with her funds, she has only 100 cNUMA and 0 rETH debt. When she tries to redeem 100 cNUMA since she has 0 collateral balance (0 rETH) she will not be able to redeem the 100 cNUMA and it will be stuck in contract.
### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/lending/NumaComptroller.sol#L896C11-L999

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/lending/NumaComptroller.sol#L270-L301
### Impact
When a user repays his entire debt, he will have no collateral balance. Hence, user wont be able to withdraw its all cToken balance!
### Recommendation
...
- [ ] Fixed
- [ ] Will not fix
- [x] In discussions

## [H-11] The contract can be permanently stuck if an attacker repays the entire debt, leaves a minuscule debt, and then repays the rest in a separate transaction

### Summary
Due to rounding in Solidity, someone can permanently lock the contract by repaying the entire (debt - 1) and then repaying the remaining amount. This action will not decrease the `rwdFromDebt` but will reduce the debt to "0." As a result, `debt - rwdFromDebt` will always underflow, and the contract will become inoperable because extracting rewards is the first operation performed in any function call.

### Vulnerability Details
First, the attacker notices that there is some debt in the Numa vault and proceeds as follows:

**Assume the following before the example:**

- `debt`: 100 * 1e18 + 9
- `rwdFromDebt`: 12345129

The attacker repays `debt - 2` amount of debt. Since this is less than the total debt, all the repaid debt will first go to the vault.

Inside the `NumaVault::repay()` function, `rewardsFromDebt` will be decreased by:

uint extractedRwdFromDebt = FullMath.mulDiv(rewardsFromDebt, _amount, debt);

In Solidity, this will be:

12345129 * (debt - 2) / debt

When we decrease this from the `rwdFromDebt`, the result will be 1 wei.

Following the execution of `NumaVault::repay()`, the debt will be decreased by the amount repaid:

debt = debt - _amount;

In Solidity, this will be 2 wei.

**Now, after this action:**

- `rwdFromDebt`: 1
- `debt`: 2

Next, the attacker will repay the remaining 1 wei worth of debt.

This time, `rwdFromDebt` will be calculated as:

1 * 1 / 2

which is "0" in Solidity. However, the debt will be decreased, resulting in `1 - 1 = 0`.

**In the end:**

- `debt`: 0
- `rwdFromDebt`: 1

This will cause underflows when the main flywheel functions are called in `NumaVault::rewardsValue()`. The vault will be forever blocked, and almost none of the functions will be able to work.

**You can copy this to Remix and run to see the rounding errors described above:**

function letsDoNuma() external pure returns (uint, uint, uint) {
        uint debt = 100 * 1e18 + 9;
        uint amount = debt - 2;
        uint rwdFromDebt = 12345129;

        uint deduct = Math.mulDiv(rwdFromDebt, amount, debt); 


        return (rwdFromDebt - deduct, debt - amount, Math.mulDiv(1, 1, 2));
    }

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L835-L870

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L320
### Impact
Forever DOS of vault
### Recommendation
tbd
- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions

## [H-12] Wrong denomination of units when calculating users liquidity in NumaComptroller

### Summary
Difference units are compared when NumaComptroller calculates the accounts net liquidity

### Vulnerability Details
Assume the `collateral` is rETH and the `borrow` is NUMA.

According to `NumaComptroller::getHypotheticalAccountLiquidityIsolateInternal` and `NumaPriceOracleNew::getUnderlyingPriceAsCollateral`, `vars.sumCollateral` will be in rETH terms.

Then, `vars.oraclePriceBorrowed` will be in NUMA terms, which means `vars.sumBorrowPlusEffects` will be in NUMA terms.

Finally, this check will never be accurate since it compares NUMA and rETH terms:

if (vars.sumCollateral > vars.sumBorrowPlusEffects) {
    return (Error.NO_ERROR, vars.sumCollateral - vars.sumBorrowPlusEffects, 0, 0);
}

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/lending/NumaComptroller.sol#L896-L999

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/lending/NumaPriceOracleNew.sol#L25-L64

### Impact
Accounts liquidity are calculated completely off
### Recommendation
tbd
- [ ] Fixed
- [ ] Will not fix
- [x] In discussions

## [H-13] Users can borrow more than their collateral due to previous borrow balance is not accounted into liquidity calculation

### Summary
Accounts previous borrow balance is not added to the new borrow/redeem balance when borrowing or redeeming.

### Vulnerability Details
`vars.sumBorrowPlusEffects` is only considering the current borrowed amount but not counts the previous borrow balance of the user.

`vars.borrowBalance` is never used, it should first calculate the previous borrows and add the new borrow/redeem amount on top of it.

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/lending/NumaComptroller.sol#L896-L999

### Impact
Accounts can borrow more than their liquidity up to immense levels of undercollateralization.
### Recommendation
add the following line:

+       vars.sumBorrowPlusEffects = mul_(vars.oraclePriceBorrowed, vars.borrowBalance);
        vars.sumBorrowPlusEffects = mul_ScalarTruncateAddUInt(vars.tokensToDenomCollateral, redeemTokens, vars.sumBorrowPlusEffects);
        vars.sumBorrowPlusEffects = mul_ScalarTruncateAddUInt(vars.oraclePriceBorrowed, borrowAmount, vars.sumBorrowPlusEffects);

- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions

## [M-1] Sell fee rewards sent to reward address can be higher than the total sell fee amount

### Summary
When a user sells NUMA for rETH, the rETH received and the sell fee in rETH are not segregated in the contract, which can result in the actual fee taken being higher than the total sell fee intended in rETH.

### Vulnerability Details
Say a user sells 100 NUMA and the amount of rETH calculated is 1 rETH.

`tokenAmount` will be 1.

Then, assume the sell fee is 950, which means that the user received rETH will be:

`_tokenOut = (tokenAmount * fee) / BASE_1000;`
which is $1 * 950 / 1000 = 0.95$ rETH.

That means the user will receive 0.95 rETH and 0.05 rETH will be the total sell fee taken.

Then, the fee taken is calculated as:
`uint256 feeAmount = (fees * tokenAmount) / BASE_1000;`

`fees` value is a value settable by the admin of the contract and currently set to "10"
`tokenAmount` was the entire amount before the sell fee, which is 1 rETH.

$( 10 * 1 / 1000 = 0.01 ) rETH$ will be the fee. The remaining 0.04 rETH will stand in the vault, which is not a problem.

Now, assume the sell fee was 995 after an admin update.

`_tokenOut` would be:
$( 1 * 995 / 1000 = 0.995 ) rETH$ and 0.005 rETH would be the fee.

However, the fee calculation is still the same which is:
$( 10 * 1 / 1000 = 0.01 ) rETH$, which is way higher than the entire sell fee!
### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L576-L660
### Impact
Owner of the contract can set the fees to reasonable values and escalate that but I wanted to write down this as an issue since there can be loss of funds if its mistakenly updated as described in the above example
### Recommendation
I think the best would be the take the fee as a percentage of the buy/sell fee instead of a fixed amount.

If the sell_fee is 0.05 rETH, instead of taking a fixed 0.01 rETH, maybe taking 40% of the sell/buy fee would work and would be more autonomus as well. This would also mean that when the sell_fee is high due to scaling, the fees collected will also be relatively higher.

If this is not ok, I suggest changing the `sell_fee` value in VaultManager and `fees` variable in NumaVault simultaneously to have a clearer and safer admin functionality
- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions
## [M-2] The execution flow in buy/sell NUMA is incorrect

### Summary
When users buy or sell, the rewards are not taken as first step which can also update the lst token value if it rebased. However, the current implementation is not following the correct flow described below.

### Vulnerability Details
Every new buy/sell should respect the latest LST token value and update it if necessary (i.e., if 1 day has passed and rewards have accrued for rETH). However, the current implementation first updates the vault, which calculates the synth/vault's total ETH value using an outdated rETH/ETH value.

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L400-L420

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L485-L504

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L576-L593
### Impact
Unfair buy/sell and interest rate calculations
### Recommendation
The correct flow should be:
1. Accrue interest in CToken.
2. Extract rewards (possibly update the rETH value and take fees).
3. `vault.updateAll()`.
- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions
## [M-3] Liquidator can be the borrower which is against the Compound code

### Summary
When an account is liquidated by a liquidator, Compound forces that the liquidator is not the borrower to have a smooth incentive flywheel and code logic. However, this is not enforced in current code

### Vulnerability Details
Since the `msg.sender` will always be the vault, liquidator can be the borrower.

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L979-L1039

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/lending/CToken.sol#L788-L790
### Impact
Code that is not allowed in Compound, unfair liquidation incentives and balance tracking
### Recommendation
add the following check

function liquidateNumaBorrowerFlashloan(address _borrower,uint _numaAmount) external whenNotPaused {
    require(msg.sender != _borrower, "cant liquidate your own position");
}

- [x] Fixed
- [ ] Will not fix
- [ ] In discussions
## [M-4] `liquidateLstBorrowerNoSwap` forgets to handle the excess NUMA in Vault contract

### Summary
When an account is liquidated and the profit is capped, the excess NUMA will be idle in the Vault contract

### Vulnerability Details
Assume:

NUMA received after liquidations 120
NUMA debt was 100
liquidatior profit is 20
assume that the max NUMA profit is 15, which means user will receive 115 NUMA and 5 NUMA will be idle in contract.

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L1317-L1388
### Impact
Idle NUMA in Vault contract though it can be withdrawn to admin via `withdrawToken` function
### Recommendation
If its intended and to be sweeped later on, then directly send it to admins address. Alternatively, excess NUMA can be burnt.
- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions
## [M-5] `liquidateLstBorrowerNoSwap` will always leave a small amount of debt behind

### Summary
Because the interest is not updated, `borrowBalanceStored` will not be the actual borrow balance of user hence, there will be a small debt leftover after liquidating the bad debt.

### Vulnerability Details
As we can see in Compound's `cToken::borrowBalanceStoredInternal()` function, the borrow balance is the latest balance before the most recent interest accrual. Because of this, the debt that will be liquidated will not be the actual amount, and there will always be a small remaining debt after the liquidation.
### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/lending/CToken.sol#L253-L278

https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L934
### Impact
Small debt will be stuck in NumaVault. If the interest rate is high and its not called for a long time then this amount could be bigger
### Recommendation
Accrue interest at the beginning of the `liquidateBadDebt` function or even better, use `borrowBalanceCurrent` to get the entire debt of user
- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions
## [Informational - 1] Synth value higher than vaults total ETH is not an extreme scenario

### Summary
When the synth balance in ETH is higher than the vault's total ETH balance, the code assumes this scenario as an extreme case and sets the NUMA price to a very small amount to encourage users to mint NUMA. However, this can be extremely unfair, and this occurrence is not necessarily an extreme case.

### Vulnerability Details
Since there can be many nuAssets like nuBTC, nuGOLD, and nuX, and the values of these assets can increase significantly, the overall nuAsset balances in ETH can become higher than the total ETH held by the vault.

In this scenario, the amount of NUMA minted can be excessively high depending on the ratios.

Let's do an example:
- NUMA circulating supply: 1000
- Vault's total ETH balance: 100
- Synth's total ETH balance: 101

Since the synth's total ETH balance is higher, the alternative formula will be used in the `VaultManager::tokenToNuma` and `VaultManager::numaToToken` functions:

This is the special formula when the synth's total ETH value is higher than the total ETH balance of the vault:

result = FullMath.mulDiv(
                EthValue,
                1 ether, // 1 ether because NUMA has 18 decimals
                minNumaPriceEth
            );

According to this formula, the amount of NUMA minted for 1 ETH worth of rETH would be:
$1*0.000001 = 1,000,000 NUMA$

As we can see, this results in an excessively high amount of NUMA. The likelihood of the synth balance in ETH being higher than the vault's total balance in ETH is not that extreme since nuAssets can appreciate in ETH value significantly.
### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/VaultManager.sol#L467-L497
### Impact
....
### Recommendation
...
- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions

## [I-2] Liquidations of NUMA can be impossible if cLST token is highly utilized

### Summary
When NUMA positions are liquidated the seized amount of cToken is forced to be the cLST token. This cLST token that seized is withdrawn from Compound inside the liquidation flow. However, if the cLST token is not liquid the withdrawal from cLST to LST will be impossible hence, the liquidation will not be possible.

### Vulnerability Details
Inside the liquidate functions of Vault the seized tokens are always withdrawn from Compound to its underlying:

        cNuma.liquidateBorrow(_borrower, _numaAmount,CTokenInterface(address(cLstToken))) ;

        // we should have received crEth with discount
        // redeem rEth
        uint balcToken = IERC20(address(cLstToken)).balanceOf(address(this));

        uint balBefore = IERC20(lstToken).balanceOf(address(this));
        cLstToken.redeem(balcToken); 
        uint balAfter = IERC20(lstToken).balanceOf(address(this));
        uint receivedlst = balAfter - balBefore;

However, if the cToken's are not liquid enough then the redeeming from cToken to its underlying will not be possible

### Links to Code
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L979-L1388
### Impact
Late liquidations, possibly bad debt
### Recommendation
Give user an option to choose which cToken to seize the tokens. It could be cNUMA or the cLST.

Also, if the NUMA is not flashloan minted, then give user the cNUMA or cLST
- [ ] Fixed
- [ ] Will not fix
- [ ] In discussions
## Low Severity Issues and Best Practices
* Use solidity [prettier](https://github.com/prettier-solidity/prettier-plugin-solidity)
- [ ] Fixed
- [ ] Will not fix
* Use solidity [linter](https://www.immunebytes.com/blog/using-solhint-ethereums-solidity-linter/)
- [ ] Fixed
- [ ] Will not fix
* Function names start with lower case and follows the camel case pattern (i.e: `getMaxBorrow()`, `numaPriceToEth()`)
- [ ] Fixed
- [ ] Will not fix
* Use camel case on storage variables as well for example on:
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/VaultManager.sol#L55-L74
- [ ] Fixed
- [ ] Will not fix
* Change the `_amount` input variable name to `_ethAmount` and change the `EthValue` return variable name to `tokenAmount`
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/libraries/OracleUtils.sol#L59-L96
- [ ] Fixed
- [ ] Will not fix
* Synths might change if these would be called with state update hence, the price would be different
https://github.com/NumaMoney/Numa/blob/1a760477e74762619899e21e5ca937c57d4d0b63/contracts/NumaProtocol/NumaVault.sol#L756-L798
- [ ] Fixed
- [ ] Will not fix