# NumaV2 Security Review // 06-02-2024

### Scope
| Type | File | Logic Contracts | Interfaces | Lines | nLines | nSLOC | Comment Lines | Complex. Score | Capabilities |
| ---- | ------ | --------------- | ---------- | ----- | ------ | ----- | ------------- | -------------- | ------------ |
| 📝 | nuAssets/nuAssetManager.sol | 1 | **** | 114 | 109 | 58 | 29 | 48 | **** |
| 📝 | libraries/OracleUtils.sol | 1 | **** | 41 | 39 | 29 | 5 | 26 | **** |
| 📝 | NumaProtocol/VaultManager.sol | 1 | **** | 234 | 222 | 135 | 50 | 113 | **** |
| 📝 | NumaProtocol/VaultOracle.sol | 1 | **** | 58 | 55 | 32 | 10 | 31 | **** |
| 📝 | NumaProtocol/NumaVault.sol | 1 | **** | 425 | 408 | 240 | 109 | 201 | **<abbr title='Uses Assembly'>🖥</abbr>** |
| 📝 | **Totals** | **5** | **** | **872** | **833** | **494** | **203** | **419** | **<abbr title='Uses Assembly'>🖥</abbr>** |

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

## [H-1] Whitelisted addresses can alter the price of NUMA and drain the ETH in the vault
### Summary
Whitelisted addresses NUMA tokens are not counted as circulating hence, if the whitelisted address goes malicious they can take advantage of it and drain the vaults ETH.
### Vulnerability Details
The circulating NUMA is calculated by subtracting the total supply of NUMA from the balance held by whitelisted addresses. This calculation relies on the `balanceOf` function from the ERC20 standard, as shown below:

function getNumaSupply() public view returns (uint)
    {
        uint circulatingNuma = numa.totalSupply(); 
      
        uint256 nbWalletsToRemove = removedSupplyAddresses.length();
        require(nbWalletsToRemove < max_addresses,"too many wallets to remove from supply");
        // remove wallets balances from numa supply
        for (uint256 i = 0; i < nbWalletsToRemove;i++)
        {
            uint bal = numa.balanceOf(removedSupplyAddresses.at(i));
            circulatingNuma -= bal;
        }
        
        return circulatingNuma;
    }

Now, let's consider an example of how an attack could occur:

**Textual PoC:**
Assume:
1 ETH is equivalent to 40,000 NUMA.
The total circulating NUMA is 4 million.
Alice is one of the whitelisted addresses.
The trader holds 2 million NUMA.
The vault's total ETH balance is 200.
The synthetics' total ETH balance is 100.

Under normal circumstances, if the trader were to sell their 2 million NUMA, they would receive:

**2M * (200-100) / 4M = 50 ETH which is inline because 2M NUMA in external market also prices as 50 ETH.**

However, the trader could attempt to manipulate the system as follows:
They send 1 million NUMA to Alice, reducing the circulating supply to 3 million, and then redeem the remaining 1 million NUMA for LST.

**1M * (200 - 100) / 3M = 33.33333333 ETH assuming a 5% sell fee that would be 31.66666667 ETH worth rETH sent to the trader.**

Then the trader initially sent 1M NUMA to Alice which was valuated at 25 ETH. If Alice and trader co-operates, then the total ETH is 31.66666667 + 25 = 56.66666 which means total 6.666 ETH profit. Alice and trader can use this as the profit of their malicious action.

Also, in general, sending tokens to any whitelisted address or the whitelisted addresses sending tokens to others will alter the price in vault which is heavily dependent on the trusteness of these addresses.

For example, NUMA cant' be used as collateral in an market because of this, someone can send the NUMA to the whitelisted address to decrease the price of NUMA in the vault and in general overall in market and liquidate someone that using NUMA as collateral.

Also, the whitelisted addresses should not be able to use the vaults functions because their NUMA balance will not be in circulated amount they will have a imbalanced advantage over NUMA vault operations over other users.
### Impact
The breaking of the invariant occurs when the circulating supply of NUMA decreases or increases, which should reflect in the synthetics' ETH value. This change in circulating supply typically happens when NUMA is either burnt directly in the vault or bought from the vault. However, despite the trustworthiness of whitelisted addresses, they can still receive NUMA tokens that are not counted as circulating supply. Consequently, the price of NUMA decreases in the vault.

While this may not be profitable for a NUMA airdropper to a whitelisted address, it poses significant challenges for extended use cases of NUMA, such as collateral for a money market or a Chainlink price feed to price NUMA. These use cases would not be feasible due to this effect.

Even if the whitelisted addresses are trusted, this issue can still be of concern, considering the potential draining of the vault.
### Recommendation
1- Do not use "balanceOf" for accounting how much tokens are not circualted for these addresss. Instead, use a value in storage and decrease/increase the value by the previleged owner address in the VaultManager contract.

2- Do not allow the whitelisted address to interact with the vault contract.

- [ ] Fixed
- [x] Will not fix
## [M-1] Incorrect decimal scaling in OracleUtils
### Summary
Decimals are incorrectly scaled when deriving the price of the underlying token
### Vulnerability Details
Function getPriceInEth in the OracleUtils function is responsible for deriving the price of the underlying token to ETH in Wei (18 decimals). However, the return value can be more or lesser than 18 decimals.

First, the function gets the input token amount as `_amount` which is in the decimals of the underlying token.

Then, the decimals are querried from the chainlink oracle as follows:
`uint256 decimalPrecision = AggregatorV3Interface(_pricefeed).decimals();`
Note that, this decimals value is the decimals of the "answer" value that chainlink returns. Not the underlying tokens decimals!

Then, according to the quote/base tokens the following code executed to determine the final result of price in ETH:

if (ethLeftSide(_pricefeed)) 
        {
            EthValue = FullMath.mulDiv(_amount, 10**decimalPrecision,uint256(price));
        }
        else 
        {
            EthValue = FullMath.mulDiv(_amount,uint256(price), 10**decimalPrecision);
        }

The problem is that if the underlying token is not in 18 decimals the result value will not be 18 decimals aswell.

**Textual PoC:**
Assume that the underlying token has 8 decimals. Hence, the _amount will be in 8 decimals.
Chainlink returns the price in 18 decimals for token/ETH pair.
Then the EthValue will be calculated as:
"_amount * decimalPrecision / price" which if we only account the decimals it would be:
" 8 + 18 - 18" which would give us 8. Hence, the resulting value will be in 8 decimals, not in 18 decimals as we value the ETH.
### Impact
Any tokens that does not have 18 decimals will fail to calculate the result in 18 decimals which the execution of the vault will not work as intended.
nuAssets has fixed 18 decimals so that shouldnt be a problem for them
Majority of the LST tokens are in 18 decimals aswell.
Using a token that is not in 18 decimals is very unlikely but if its ever used then the calculation will not be accurate.
### Recommendation
Add the decimal difference scales up to 18 from the underlying token

EthValue = FullMath.mulDiv(_amount, 10**decimalPrecision,uint256(price));
EthValue = EthValue * 10**(18 - IERC20Metadata(_token).decimals());

- [x] [Fixed](https://github.com/NumaMoney/Numa/commit/f2ec8bfd2cfeb7409c4e486a53838f0d4774188f)
- [ ] Will not fix

## [M-2] getDecayDenominator() is not time sensitive enough
### Summary
The decay denominator calculation can round down, leading to unfair decaying.
### Vulnerability Details
When calculating the decay denominator the actual value can round down.
`(1000 * delta_s) / period;`
considering period is 90 days in seconds, delta_s needs to be higher than 77760 (approx 2.12 hours) seconds to give the correct decaying result.
Which means a user depositing to the vault and another user depositing after 2 hours when decaying is open will get the same decaying factors, although there is a significant amount of time passed.

uint256 currenttimestamp = block.timestamp;
uint256 delta_s = currenttimestamp - decaytimestamp;
// should go down to 1 during 90 days
uint256 period = 90 * 1 days; 
uint256 decay_factor_1000 = (1000 * delta_s) / period;

### Impact
Unfair decaying
### Recommendation
Scale the getDecayDenominator() value by 1e18. When calculating the "result" in TokenToNuma and NumaToToken do the math operations in 18 decimals instead of 1000 to increase the precision.

- [ ] Fixed
- [x] Will not fix

## [M-3] Lack of Chainlink feed validations
### Summary
Chainlink feed validations are not enough and not in security best practices.
### Vulnerability Details
Chainlink price feeds lack staleness checks and sequencer uptime checks.

1- Lack of staleness check:
Chainlink oracles sometimes fail to return the price within the determined heartbeat interval. If such a scenario happens, the outdated price will be used to price the assets in the Numa system.

2- Lack of sequencer checks:
When the sequencer is down for some reason, the Chainlink price feed will not be updated. In such a case, someone can interact with the protocol from L1 directly, bypassing the sequencer. Since the sequencer is down, the Chainlink price feed will not be updated, and the user will be able to use the outdated price.

### Impact
The stale price can be used
### Recommendation
Sequencer best practice:
https://docs.chain.link/data-feeds/l2-sequencer-feeds#example-code

Price staleness and general best practice:

function getOraclePrice(IAggregatorV3 _priceFeed, int192 _maxPrice, int192 _minPrice) public view returns (uint256 ) {
        (
            uint80 roundID,
            int signedPrice,
            /*uint startedAt*/,
            uint timeStamp,
            uint80 answeredInRound
        ) = _priceFeed.latestRoundData();
        //check for Chainlink oracle deviancies, force a revert if any are present. Helps prevent a LUNA like issue
        require(signedPrice > 0, "Negative Oracle Price");
        require(timeStamp >= block.timestamp - HEARTBEAT_TIME , "Stale pricefeed");
        require(signedPrice < _maxPrice, "Upper price bound breached");
        require(signedPrice > _minPrice, "Lower price bound breached");
        require(answeredInRound >= roundID, "round not complete");

        uint256 price = uint256(signedPrice);
        return price;
    }

- [x] [Fixed](https://github.com/NumaMoney/Numa/commit/4a90f63de37557702527cd8ef9f39ebd34d0b8aa)
- [ ] Will not fix

## [M-4] LST price should be updated every 24 hours regardless of reward threshold
### Summary

### Vulnerability Details
When 24 hours have passed since the last rewards were extracted, new rewards can be extracted. However, rewards are only extracted if they exceed a certain threshold called `rwd_threshold`. The calculation of the reward threshold is adjustable by the vault owner. The calculation of the rewards is as follows:

function rewardsValue() public view returns (uint256,uint256)
    {
        require(address(oracle) != address(0),"oracle not set");        
        uint currentvalueWei = oracle.getTokenPrice(address(lstToken),decimals); 
        if (currentvalueWei <= last_lsttokenvalueWei)   
        {
            return (0,currentvalueWei);   
        }
        uint diff = (currentvalueWei - last_lsttokenvalueWei);
        uint balance = lstToken.balanceOf(address(this));
        uint rwd = FullMath.mulDiv(balance, diff, currentvalueWei);
        return (rwd,currentvalueWei);   
    }

So basically, the condition for extracting rewards is that
**balance * exchangeRateDiff / currentExchangeRate**
must be greater than the threshold. However, this mathematical operation can result in a value lower than the reward threshold in two cases:

1- The LST rebasing is very small, resulting in low rewards.
2- The amount of LST the vault holds is very small, such that the new rebasing is insufficient.
If the second case occurs, there can be a significant change in the LST exchange rate, which is also used in "buy/sell" functions where NUMA heavily depends. However, just because the reward threshold is not satisfied, the price will also not be updated, which is incorrect.

### Impact
Since reward threshold is settable by the admin it is not a big issue as long as it is set to very small number. However, regardless of the reward threshold amount, the feed should be updated every 24 hours for the core functions of vaults "buy/sell"
### Recommendation
Set the reward threshold to "0" as default or get rid of it. If the rewards collected are lower than expected (threshold) then handle that case inside the rewardRecipient contract here
`receiver.DepositFromVault(rwd)`
only check if the rwd > 0, dont check if its higher than a value.
- [x] [Fixed](https://github.com/NumaMoney/Numa/commit/4a90f63de37557702527cd8ef9f39ebd34d0b8aa)
- [ ] Will not fix

## [M-5] Slashing of the LST may result in users minting undercollateralized NUMA tokens
### Summary
When a slashing event affects rETH, its value relative to ETH decreases. Consequently, the `lastvalueinwei` variable within the contract remains unchanged, incorrectly assuming the pre-slashing value of rETH.
### Vulnerability Details
Assuming that 1 rETH equals 1 ETH, let's consider the scenario where an ETH slashing event occurs, reducing the value of 1 rETH to 0.9 ETH.

Within the contract, we observe that the `last_lsttokenvalueWei` is only updated when there are profits in rETH, and it does not account for potential slashing events.

The `last_lsttokenvalueWei` is only updated within the `extractInternal` function when the reward value is non-zero:

function rewardsValue() public view returns (uint256,uint256)
    {
        require(address(oracle) != address(0),"oracle not set");        
        uint currentvalueWei = oracle.getTokenPrice(address(lstToken),decimals); 
        // if there is an slashing event this will return "0"
        if (currentvalueWei <= last_lsttokenvalueWei)   
        {
            return (0,currentvalueWei);   
        }
        uint diff = (currentvalueWei - last_lsttokenvalueWei);
        uint balance = lstToken.balanceOf(address(this));
        uint rwd = FullMath.mulDiv(balance, diff, currentvalueWei);
        return (rwd,currentvalueWei);   
    }

function extractRewardsNoRequire() internal
    {
        //rewards address has to be specified
        if (RWD_ADDRESS != address(0))
        {
            if (block.timestamp >= (last_extracttimestamp + 24 hours))
            {
                (uint256 rwd,uint256 currentvalueWei) = rewardsValue();
                if (rwd > rwd_threshold) 
                {
                     extractInternal(rwd,currentvalueWei);
                }
            }
        }
    }

This means that until the rETH recovers back to 1 ETH, the vault will assume 1 rETH is equal to 1 ETH and will never update its "-`last_lsttokenvalueWei`.

**Textual PoC:**
Consider the following scenario:

Circulating NUMA: 4M
1 rETH is valued at 0.9 ETH in the external market
ETH balance of the vault: 200
Synth balance of the vault: 100
A user provides 1 rETH, which is valued at 1 ETH by the vault, despite being worth only 0.9 ETH externally. The NUMA to be minted is then calculated as:

1 * 4M / 200-100 = 40000

Thus, 40000 NUMA will be minted to the user, equivalent to 1 ETH. However, only 0.9 ETH worth of rETH was used for this minting, as the vault is unaware of the updated value due to the `last_lsttokenvalueWei` not being updated.
### Impact
It is crucial that the vault is always uses the chainlink feed to value rETH such that the arbs can happen between the price feed and the external market. However, in this case, the price feed will never be used and the vault will continously misvalue the rETH such that NUMA tokens that will be minted via vault will be cheaper.

However, NUMA uses rETH price to simply refer to it not stricly pegged to rETH. Hence, the medium severity is fair.
### Recommendation
tbc

- [ ] Fixed
- [x] Will not fix

## Low Severity Issues and Best Practices

* Use solidity prettier
https://github.com/prettier-solidity/prettier-plugin-solidity

* Inconsistent require
Usually the minimum amount is the amount that can be deposited without reverts. The "MIN" value is 1000 but the actual "MIN" value is 999 because of the require does revert on equality.
I'd advise to make it >= for clarity.
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/NumaVault.sol#L283
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/NumaVault.sol#L322
- [x] Fixed
- [ ] Will not fix

* Function name starts with capital letter
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/NumaVault.sol#L183-L188
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/VaultManager.sol#L98-L129
- [x] Fixed
- [ ] Will not fix

* Use CAPITALS on only immutables and constants
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/NumaVault.sol#L23-L35
- [x] Fixed
- [ ] Will not fix

* Unnecessary check
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/nuAssets/nuAssetManager.sol#L108
This check is ensured in the "addNuAsset" such that there will be never more assets than max
- [x] Fixed
- [ ] Will not fix

* New decay is possible while previous decay is still on
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/VaultManager.sol#L51-L55
I think it is best to not be able to start a decay when the previous decay is still running.
- [ ] Fixed
- [x] Will not fix

* Add a function here that checks VaultManager added this as a vault. Because the flow is as follows: Deploy vault, set vault in manager, set manager in vault. If the vaults manager is set before the vaults manager sets the vault as a registered vault, then in that small period of time someone can mint NUMA tokens without vault manager knowing the vault.
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/NumaVault.sol#L126-L131
**Recommendation:**

function setVaultManager(address _vaultManager) external onlyOwner  
    {
        require(_vaultManager != address(0x0),"zero address");
        // add the "isVault" external view function to the vault manager contract
        require(IVaultManager(_vaultManager).isVault(address(this)), "not a registered vault");
        vaultManager = IVaultManager(_vaultManager);
        emit SetVaultManager(_vaultManager);
    }

- [ ] Fixed
- [x] Will not fix

* Add minBuy/minSell amounts as inputs given from user. Since this is Arbitrum the frontrunning and unfortunate tx ordering in a block is very hard. That's why I label this as low. However, although it's arbitrum there is still a chance this could go wrong. Also, in future, Arbitrum might decentralise the sequencer which MEV can be threat and the the frontrunning would be a thread aswell. Also, if the contracts will ever be deployed to a L1, then this would be a very big problem.
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/NumaVault.sol#L281-L354
**Recommendation:**

function buy(uint _inputAmount, uint _minNumaAmount, address _receiver) external nonReentrant whenNotPaused 
    {   
        .
        .
        .
        .
        .
        uint numaMinted = (numaAmount * BUY_FEE) / FEE_BASE_1000;
        require(numaMinted >= _minNumaAmount, "Min NUMA");
    }

- [x] Fixed
- [ ] Will not fix

* Extract rewards before checking the "MAX"
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/NumaVault.sol#L281C5-L289
**Recommendation:**

function buy(uint _inputAmount,address _receiver) external nonReentrant whenNotPaused 
    {
        require(_inputAmount >= MIN, "must trade over min");
        -> extractRewardsNoRequire();
        
        uint256 vaultsBalance = lstToken.balanceOf(address(this));
        uint256 MAX = (MAX_PERCENT*vaultsBalance)/FEE_BASE_1000;
        -> require(_inputAmount <= MAX, "must trade under max");
        .
        .
    }

- [x] Fixed
- [ ] Will not fix
* I guess you can check for this instead of this equations and re-write the function simply:
https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/NumaVault.sol#L175-L180
`SELL_FEE + _fees <= 10_000 && BUY_FEE + _fees <= 10_000`
tho a good thing to have here is to cap these fees for the trust of the system, something like this
**Recommendation:**

require(SELL_FEE <= 1000);
require(BUY_FEE <= 1000);
require(_fees <= 2000);
require(SELL_FEE + _fees <= 10_000 && BUY_FEE + _fees <= 10_000);

- [ ] Fixed
- [x] Will not fix

https://github.com/NumaMoney/Numa/blob/4234ad8c548f78a4687e87db6e2e20af636365e0/contracts/NumaProtocol/NumaVault.sol#L175-L180
## Testing Suite

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

import {VaultOracle} from "../contracts/NumaProtocol/VaultOracle.sol";
import {NumaVault} from "../contracts/NumaProtocol/NumaVault.sol";
import {VaultManager} from "../contracts/NumaProtocol/VaultManager.sol";
import {nuAssetManager} from "../contracts/nuAssets/nuAssetManager.sol";
import {nuAsset} from "../contracts/nuAssets/nuAsset.sol";

// Openzeppelin stuff
import { IERC20Metadata } from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";

// foundry stuff
import {Test} from "forge-std/Test.sol";

contract ReviewTests is Test {

    VaultOracle public vaultOracle;
    NumaVault public rethNumaVault;
    VaultManager public vaultManager;
    nuAssetManager public nuAssetManager_;
    nuAsset public nuUSD;

    address public deployer = address(1);
    address public tapir = address(69);
    
    address private constant RETH = 0xEC70Dcb4A1EFa46b8F2D97C310C9c4790ba5ffA8;
    address private constant RETH_ETH_ORACLE = 0xD6aB2298946840262FcC278fF31516D39fF611eF; 
    address private constant NUMA = 0x7FB7EDe54259Cb3D4E1EaF230C7e2b1FfC951E9A;
    address private constant NUMA_ADMIN = 0x7B224b19b2b26d1b329723712eC5f60C3f7877E3;
    address private constant WBTC = 0x2f2a2543B76A4166549F7aaB2e75Bef0aefC5B0f;
    address private constant WBTC_ORACLE = 0xc5a90A6d7e4Af242dA238FFe279e9f2BA0c64B2e; 
    address private constant ETH_USD_ORACLE = 0x639Fe6ab55C921f74e7fac1ee960C0B6293ba612;

    function setUp() public {
        vm.startPrank(deployer);
        vaultOracle = new VaultOracle();
        vaultOracle.setTokenFeed(RETH, RETH_ETH_ORACLE);
        rethNumaVault = new NumaVault(NUMA, RETH, 18, address(vaultOracle));
        rethNumaVault.unpause();

        // nuAssets
        nuAssetManager_ = new nuAssetManager();
        nuUSD = new nuAsset();
        nuUSD.initialize("numaUSD", "nuUSD", deployer, deployer, deployer);
        nuUSD.mint(tapir, 10_000 * 1e18);
        nuAssetManager_.addNuAsset(address(nuUSD), ETH_USD_ORACLE);

        vaultManager = new VaultManager(NUMA, address(nuAssetManager_), 200); // @dev 200 as I see in other tests
        rethNumaVault.setVaultManager(address(vaultManager));
        vaultManager.addVault(address(rethNumaVault));
        vm.stopPrank();
        vm.prank(NUMA_ADMIN);
        nuAsset(NUMA).grantRole(keccak256("MINTER_ROLE"), address(rethNumaVault));
    }

    // forge test --match-contract ReviewTests --match-test test_DecayingDenominatorRoundsDown --fork-url https://arb1.arbitrum.io/rpc -vv
    function test_DecayingDenominatorRoundsDown() external {
        uint256 currentTimestamp = 100;
        uint256 decayTimestamp = 98;
        uint256 period = 90 * 1 days;
        uint256 decayingDenominator = 200;
        uint256 delta_s = currentTimestamp - decayTimestamp;
        uint256 decay_factor_1000 = (1000 * 7775) / period; // @review not enough, this can easily rounded down

        assertEq(decay_factor_1000, 0);

        uint256 currentDecay_1000 = decay_factor_1000 * 100 + (1000 - decay_factor_1000) * decayingDenominator;
        assertEq(currentDecay_1000, decayingDenominator * 1000);
    }