# Numa Vault V2 Fix Review // 17-10-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
2f38eb77bff6638f653fa068d3a4b8031a749d2a

## [M-1] cNUMA `leverageStrategy` will revert due to interest is not being updated in the beginning of the function

### Summary
If interest needs to be accrued in cNUMA, leveraging will always fail because the latest interest is never accounted for. The require check strictly ensures that the borrowed amount matches both the previous borrow balance and the new borrow balance.

### Vulnerability Details

The `leverageStrategy()` function in `CNumaToken` fails when borrowing due to the interest accrual mechanics. The issue arises because of the way the borrow balance is checked. When the function calls `borrowInternalNoTransfer`, it triggers `accrueInterest`, which updates the user's borrow balance to include any accrued interest. This means the initial cached value of `accountBorrowBefore` becomes outdated since it doesn't account for the newly accrued interest.

Thus, when the check compares `(accountBorrows[msg.sender].principal - accountBorrowBefore)` with `borrowAmount`, it fails because the new borrow balance includes the interest, while the cached `accountBorrowBefore` doesn't. This mismatch causes the `require` condition to fail, even though the borrow itself might have been correct if the interest had been accounted for upfront.

uint accountBorrowBefore = accountBorrows[msg.sender].principal;
        // borrow but do not transfer borrowed tokens
        borrowInternalNoTransfer(borrowAmount, msg.sender);
        //uint accountBorrowAfter = accountBorrows[msg.sender].principal;
        require(
            (accountBorrows[msg.sender].principal - accountBorrowBefore) == borrowAmount,
            "borrow ko"
        );

### Links to Code
https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/lending/CToken.sol#L336-L354

https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/lending/CNumaToken.sol#L196-L203

https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/lending/CToken.sol#L769-L783

### Impact
Interest has to be updated in the same call by the user or the transaction will revert.
### Recommendation
Accrue interest in the beginning of the function if its cNUMA.
- [x] Fixed
- [ ] Will not fix
- [ ] In discussions

## [M-2] Closing a leverage position can revert due to unaccrued interest in cNUMA

### Summary
When closing leverage, the calculation to determine the required amount of cTokens to be taken from the user can fail if interest is not accrued in the `cNUMA` token.

### Vulnerability Details
The amount of `cNUMA` to be taken from the user is calculated as follows:

function closeLeverageAmount(
        CNumaToken _collateral,
        uint _borrowtorepay,
        uint _strategyIndex
    ) public view returns (uint, uint) {
        INumaLeverageStrategy strat = INumaLeverageStrategy(
            leverageStrategies.at(_strategyIndex)
        );
        // amount of underlying needed
        uint swapAmountIn = strat.getAmountIn(_borrowtorepay, true);

        // amount of ctokens to redeem this amount
        Exp memory exchangeRate = Exp({
            mantissa: _collateral.exchangeRateStored()
        });

        uint cTokenAmount = div_(swapAmountIn, exchangeRate);
        return (cTokenAmount, swapAmountIn);
    }

Note that this `cToken` value will always be slightly lower because the latest interest is not forcefully accrued.

### Links to Code
https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/lending/CNumaToken.sol#L245-L263

https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/lending/CNumaToken.sol#L300-L304

### Impact
Users can close semi positions or not close the positions at all
### Recommendation
Accrue interest in the cNUMA at the beginning of the function if its crETH.
- [x] Fixed
- [ ] Will not fix
- [ ] In discussions

## Low Severity Issues and Best Practices
* This looks unnecessary you can just use `minterContract.mint()` https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/NumaProtocol/NumaVault.sol#L1317-L1321

* This looks unnecessary you can just use `SafeERC20.safeTransfer(lstToken, msg.sender, _amount);`
https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/NumaProtocol/NumaVault.sol#L1329-L1332

* This can be deleted since you are already checking this condition in the `CNumaLst::borrowFreshNoTransfer()`
https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/NumaProtocol/NumaVault.sol#L942-L951

* `ratio2` `denominator2` can be deleted as well as console.log. What were you calculating with these?
https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/lending/NumaComptroller.sol#L1510-L1543

* Why making an external call to self? Can just use sell since its a public function? EDIT: I see now, to make the msg.sender address(this), not sure if its gas worth
https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/NumaProtocol/NumaVault.sol#L1238

* Do an if/else here to save gas instead because you are loading from storage if the else block is correct
https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/NumaProtocol/NumaVault.sol#L997-L1005

* Remove this function, `updateDebasings` can be called instead, its also not suitable since extracting rewards will decrease the LST balance hence the `getMaxBorrow()` (rename this to that pls) so an interest accrual is a must
https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/NumaProtocol/NumaVault.sol#L342-L355

* You can do if `(address(cNuma) == cNuma)` instead of byte checking which is gas extensive
https://github.com/NumaMoney/Numa/blob/5bb9180eac7241d348cd2d5a235c8dc75f196c65/contracts/lending/NumaPriceOracleNew.sol#L25

* Since aggregator decimals are constants you should also hardcode them instead of getting them by `.decimals()` everytime
https://github.com/NumaMoney/Numa/blob/99efba33cd91cdf46cbd286314e4157555aecbf1/contracts/NumaProtocol/USDCToEthConverter.sol#L166-L173

* `decimals` storage value can be constant or even be removed since the decimals is already known. Also, the chainlink feeds can be immutable or constant as well.
https://github.com/NumaMoney/Numa/blob/99efba33cd91cdf46cbd286314e4157555aecbf1/contracts/NumaProtocol/USDCToEthConverter.sol#L9-L15

* Doubt there would be any token that has more than 18 decimals. I think this part can be deleted
https://github.com/NumaMoney/Numa/blob/99efba33cd91cdf46cbd286314e4157555aecbf1/contracts/NumaProtocol/USDCToEthConverter.sol#L101