Merge pull request #124 from Ion-Protocol/jun/oz-vault-upgrade-audit-…

…merge

[OZ] Vault and Core Upgrade Audit Merge

.github/workflows/checks.yml

@@ -55,7 +55,7 @@ jobs:
         uses: codespell-project/actions-codespell@v2.0
         with:
           check_filenames: true
-          ignore_words_list: we
+          ignore_words_list: we,amountIn
           skip: ./.git,./lib,./certora
 
   validate-links:

script/deploy/vault/DeployVault.s.sol

@@ -41,8 +41,6 @@ contract DeployVault is BaseScript {
 
     bytes32 salt = config.readBytes32(".salt");
 
-    uint256 initialDeposit = config.readUint(".initialDeposit");
-
     address[] marketsToAdd = config.readAddressArray(".marketsToAdd");
     uint256[] allocationCaps = config.readUintArray(".allocationCaps");
     address[] supplyQueue = config.readAddressArray(".supplyQueue");
@@ -99,14 +97,6 @@ contract DeployVault is BaseScript {
         // require(initialDelay != 0, "initialDelay");
         require(initialDefaultAdmin != address(0), "initialDefaultAdmin");
 
-        require(initialDeposit >= 1e3, "initialDeposit");
-        require(IERC20(baseAsset).balanceOf(broadcaster) >= initialDeposit, "sender balance");
-        // require(IERC20(baseAsset).allowance(broadcaster, address(factory)) >= initialDeposit, "sender allowance");
-
-        if (IERC20(baseAsset).allowance(broadcaster, address(factory)) < initialDeposit) {
-            IERC20(baseAsset).approve(address(factory), 1e9);
-        }
-
         // The length of all the arrays must be the same.
         require(marketsToAdd.length > 0);
         require(allocationCaps.length > 0);
@@ -146,8 +136,7 @@ contract DeployVault is BaseScript {
             initialDelay,
             initialDefaultAdmin,
             salt,
-            marketsArgs,
-            initialDeposit
+            marketsArgs
         );
 
         require(vault.feeRecipient() == feeRecipient, "feeRecipient");

src/periphery/IonLens.sol

@@ -13,6 +13,13 @@ struct IlkSlot0 {
     uint48 lastRateUpdate; // block.timestamp of last update; overflows in 800_000 years}
 }
 
+/**
+ * @title Ion Lens
+ * @author Molecular Labs
+ * @notice Generalized lens contract for IonPools.
+ *
+ * @custom:security-contact security@molecularlabs.io
+ */
 contract IonLens is IIonLens {
     // --- Data ---
     struct Ilk {
@@ -93,7 +100,7 @@ contract IonLens is IIonLens {
     }
 
     /**
-     * @return The total amount of collateral in the pool.
+     * @return The total amount of collateral types in the pool.
      */
     function ilkCount(IIonPool pool) external view returns (uint256) {
         IonPoolStorage storage $ = _getIonPoolStorage();

src/token/RewardToken.sol

@@ -321,7 +321,7 @@ abstract contract RewardToken is
 
         RewardTokenStorage storage $ = _getRewardTokenStorage();
 
-        uint256 _supplyFactor = $.supplyFactor;
+        uint256 _supplyFactor = supplyFactor();
         uint256 amountNormalized = amount.rayDivDown(_supplyFactor);
 
         uint256 oldSenderBalance = $._normalizedBalances[from];

src/vault/Vault.sol

@@ -3,7 +3,6 @@ pragma solidity 0.8.21;
 
 import { IIonPool } from "./../interfaces/IIonPool.sol";
 import { RAY } from "./../libraries/math/WadRayMath.sol";
-
 import { IERC20Metadata } from "@openzeppelin/contracts/interfaces/IERC20Metadata.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import { ERC4626 } from "@openzeppelin/contracts/token/ERC20/extensions/ERC4626.sol";
@@ -49,6 +48,7 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
     error InvalidFeePercentage();
     error InvalidFeeRecipient();
     error MaxSupportedMarketsReached();
+    error InvalidIonPoolDecimals(IIonPool pool);
 
     event UpdateSupplyQueue(address indexed caller, IIonPool[] newSupplyQueue);
     event UpdateWithdrawQueue(address indexed caller, IIonPool[] newWithdrawQueue);
@@ -58,17 +58,22 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
     event FeeAccrued(uint256 feeShares, uint256 newTotalAssets);
     event UpdateLastTotalAssets(uint256 lastTotalAssets, uint256 newLastTotalAssets);
 
+    event UpdateFeePercentage(uint256 newFeePercentage);
+    event UpdateFeeRecipient(address indexed newFeeRecipient);
+    event AddSupportedMarkets(IIonPool[] marketsAdded);
+    event RemoveSupportedMarkets(IIonPool[] marketsRemoved);
+    event UpdateAllocationCaps(IIonPool[] ionPools, uint256[] newCaps);
+
     bytes32 public constant OWNER_ROLE = keccak256("OWNER_ROLE");
     bytes32 public constant ALLOCATOR_ROLE = keccak256("ALLOCATOR_ROLE");
 
     IIonPool public constant IDLE = IIonPool(address(uint160(uint256(keccak256("IDLE_ASSET_HOLDINGS")))));
 
     uint8 public immutable DECIMALS_OFFSET;
 
-    bytes32 public immutable ION_POOL_SUPPLY_CAP_SLOT =
+    bytes32 public constant ION_POOL_SUPPLY_CAP_SLOT =
         0xceba3d526b4d5afd91d1b752bf1fd37917c20a6daf576bcb41dd1c57c1f67e09;
-    bytes32 public immutable ION_POOL_LIQUIDITY_SLOT =
-        0xceba3d526b4d5afd91d1b752bf1fd37917c20a6daf576bcb41dd1c57c1f67e08;
+    bytes32 public constant ION_POOL_LIQUIDITY_SLOT = 0xceba3d526b4d5afd91d1b752bf1fd37917c20a6daf576bcb41dd1c57c1f67e08;
 
     IERC20 public immutable BASE_ASSET;
 
@@ -120,7 +125,7 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
         feePercentage = _feePercentage;
         feeRecipient = _feeRecipient;
 
-        DECIMALS_OFFSET = uint8(_zeroFloorSub(uint256(18), IERC20Metadata(address(_baseAsset)).decimals()));
+        DECIMALS_OFFSET = 4;
 
         _addSupportedMarkets(
             marketsArgs.marketsToAdd,
@@ -140,6 +145,7 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
         if (_feePercentage > RAY) revert InvalidFeePercentage();
         _accrueFee();
         feePercentage = _feePercentage;
+        emit UpdateFeePercentage(_feePercentage);
     }
 
     /**
@@ -149,6 +155,7 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
     function updateFeeRecipient(address _feeRecipient) external onlyRole(OWNER_ROLE) {
         if (_feeRecipient == address(0)) revert InvalidFeeRecipient();
         feeRecipient = _feeRecipient;
+        emit UpdateFeeRecipient(_feeRecipient);
     }
 
     /**
@@ -157,6 +164,8 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
      * address. Valid IonPools require the base asset to be the same. Duplicate
      * addition to the EnumerableSet will revert. The allocationCaps of the
      * new markets being introduced must be set.
+     * It MUST be enforced that each IonPool's RewardToken `_decimals` is equal
+     * to the decimals of this vault's base asset.
      * @param marketsToAdd Array of new markets to be added.
      * @param allocationCaps Array of allocation caps for only the markets to be added.
      * @param newSupplyQueue Desired supply queue of IonPools for all resulting supported markets.
@@ -184,13 +193,15 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
     {
         if (marketsToAdd.length != allocationCaps.length) revert MarketsAndAllocationCapLengthMustBeEqual();
 
-        for (uint256 i; i != marketsToAdd.length;) {
+        uint256 marketsToAddLength = marketsToAdd.length;
+        uint8 baseAssetDecimals = IERC20Metadata(address(BASE_ASSET)).decimals();
+        for (uint256 i; i != marketsToAddLength;) {
             IIonPool pool = marketsToAdd[i];
 
             if (pool != IDLE) {
-                if (address(pool.underlying()) != address(BASE_ASSET)) {
-                    revert InvalidUnderlyingAsset(pool);
-                }
+                if (pool.decimals() != baseAssetDecimals) revert InvalidIonPoolDecimals(pool);
+                if (address(pool.underlying()) != address(BASE_ASSET)) revert InvalidUnderlyingAsset(pool);
+
                 BASE_ASSET.approve(address(pool), type(uint256).max);
             }
 
@@ -207,6 +218,8 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
 
         _updateSupplyQueue(newSupplyQueue);
         _updateWithdrawQueue(newWithdrawQueue);
+
+        emit AddSupportedMarkets(marketsToAdd);
     }
 
     /**
@@ -229,7 +242,8 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
         external
         onlyRole(OWNER_ROLE)
     {
-        for (uint256 i; i != marketsToRemove.length;) {
+        uint256 marketsToRemoveLength = marketsToRemove.length;
+        for (uint256 i; i != marketsToRemoveLength;) {
             IIonPool pool = marketsToRemove[i];
 
             if (pool == IDLE) {
@@ -251,6 +265,8 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
         }
         _updateSupplyQueue(newSupplyQueue);
         _updateWithdrawQueue(newWithdrawQueue);
+
+        emit RemoveSupportedMarkets(marketsToRemove);
     }
 
     /**
@@ -319,8 +335,8 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
 
     /**
      * @notice Update allocation caps for specified IonPools or the IDLE pool.
-     * @dev The allocation caps are applied to pools in the order of the array
-     * within `supportedMarkets`. The elements inside `ionPools` must exist in
+     * @dev The allocation caps are applied to pools in the order of the
+     * `ionPool` array argument. The elements inside `ionPools` must exist in
      * `supportedMarkets`. To update the `IDLE` pool, use the `IDLE` constant
      * address.
      * @param ionPools The array of IonPools whose caps will be updated.
@@ -333,9 +349,10 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
         external
         onlyRole(OWNER_ROLE)
     {
-        if (ionPools.length != newCaps.length) revert IonPoolsArrayAndNewCapsArrayMustBeOfEqualLength();
+        uint256 ionPoolsLength = ionPools.length;
+        if (ionPoolsLength != newCaps.length) revert IonPoolsArrayAndNewCapsArrayMustBeOfEqualLength();
 
-        for (uint256 i; i != ionPools.length;) {
+        for (uint256 i; i != ionPoolsLength;) {
             IIonPool pool = ionPools[i];
             if (!supportedMarkets.contains(address(pool))) revert MarketNotSupported(pool);
             caps[pool] = newCaps[i];
@@ -344,6 +361,8 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
                 ++i;
             }
         }
+
+        emit UpdateAllocationCaps(ionPools, newCaps);
     }
 
     /**
@@ -365,10 +384,14 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
         uint256 totalWithdrawn;
 
         uint256 currentIdleDeposits = BASE_ASSET.balanceOf(address(this));
-        for (uint256 i; i != allocations.length;) {
+
+        uint256 allocationsLength = allocations.length;
+        for (uint256 i; i != allocationsLength;) {
             MarketAllocation calldata allocation = allocations[i];
             IIonPool pool = allocation.pool;
 
+            _supportedMarketsIndexOf(address(pool)); // Checks if the pool is supported
+
             uint256 currentSupplied = pool == IDLE ? currentIdleDeposits : pool.balanceOf(address(this));
             int256 assets = allocation.assets; // to deposit or withdraw
 
@@ -577,9 +600,6 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
     function mint(uint256 shares, address receiver) public override nonReentrant returns (uint256 assets) {
         uint256 newTotalAssets = _accrueFee();
 
-        // This is updated again with the deposited assets amount in `_deposit`.
-        lastTotalAssets = newTotalAssets;
-
         assets = _convertToAssetsWithTotals(shares, totalSupply(), newTotalAssets, Math.Rounding.Ceil);
 
         _deposit(_msgSender(), receiver, assets, shares);
@@ -797,7 +817,9 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
     }
 
     function _maxDeposit() internal view returns (uint256 maxDepositable) {
-        for (uint256 i; i != supportedMarkets.length();) {
+        uint256 supportedMarketsLength = supportedMarkets.length();
+
+        for (uint256 i; i != supportedMarketsLength;) {
             IIonPool pool = IIonPool(supportedMarkets.at(i));
 
             uint256 depositable =
@@ -837,7 +859,7 @@ contract Vault is ERC4626, Multicall, AccessControlDefaultAdminRules, Reentrancy
         (feeShares, newTotalAssets) = _accruedFeeShares();
         if (feeShares != 0) _mint(feeRecipient, feeShares);
 
-        lastTotalAssets = newTotalAssets;
+        _updateLastTotalAssets(newTotalAssets);
 
         emit FeeAccrued(feeShares, newTotalAssets);
     }

src/vault/VaultBytecode.sol

@@ -10,11 +10,13 @@ import { IERC20 } from "openzeppelin-contracts/contracts/interfaces/IERC20.sol";
  * @notice The sole job of this contract is to deploy the embedded `Vault`
  * contract's bytecode with the constructor args. `VaultFactory` handles rest of
  * the verification and post-deployment logic.
+ *
+ * @custom:security-contact security@molecularlabs.io
  */
 contract VaultBytecode {
     error OnlyFactory();
 
-    address constant VAULT_FACTORY = 0x0000000000D7DC416dFe993b0E3dd53BA3E27Fc8;
+    address public constant VAULT_FACTORY = 0x0000000000D7DC416dFe993b0E3dd53BA3E27Fc8;
 
     /**
      * @notice Deploys the embedded `Vault` bytecode with the given constructor

src/vault/VaultFactory.sol

@@ -10,6 +10,8 @@ import { SafeERC20 } from "openzeppelin-contracts/contracts/token/ERC20/utils/Sa
  * @title Ion Lending Vault Factory
  * @author Molecular Labs
  * @notice Factory contract for deploying Ion Lending Vaults.
+ *
+ * @custom:security-contact security@molecularlabs.io
  */
 contract VaultFactory {
     using SafeERC20 for IERC20;
@@ -30,7 +32,7 @@ contract VaultFactory {
         address indexed initialDefaultAdmin
     );
 
-    VaultBytecode constant BYTECODE_DEPLOYER = VaultBytecode(0x0000000000382a154e4A696A8C895b4292fA3D82);
+    VaultBytecode public constant BYTECODE_DEPLOYER = VaultBytecode(0x0000000000382a154e4A696A8C895b4292fA3D82);
 
     // --- Modifier ---
 
@@ -52,13 +54,7 @@ contract VaultFactory {
     // --- External ---
 
     /**
-     * @notice Deploys a new Ion Lending Vault. Transfers the `initialDeposit`
-     * amount of the base asset from the caller initiate the first deposit to
-     * the vault. The minimum `initialDeposit` is 1e3. If less, this call would
-     * underflow as it will always burn 1e3 shares of the total shares minted to
-     * defend against inflation attacks.
-     * @dev The 1e3 initial deposit amount was chosen to defend against
-     * inflation attacks, referencing the UniV2 LP token implementation.
+     * @notice Deploys a new Ion Lending Vault.
      * @param baseAsset The asset that is being lent out to IonPools.
      * @param feeRecipient Address that receives the accrued manager fees.
      * @param feePercentage Fee percentage to be set.
@@ -69,7 +65,6 @@ contract VaultFactory {
      * @param salt The salt used for CREATE2 deployment. The first 20 bytes must
      * be the msg.sender.
      * @param marketsArgs Arguments for the markets to be added to the vault.
-     * @param initialDeposit The initial deposit to be made to the vault.
      */
     function createVault(
         IERC20 baseAsset,
@@ -80,8 +75,7 @@ contract VaultFactory {
         uint48 initialDelay,
         address initialDefaultAdmin,
         bytes32 salt,
-        Vault.MarketsArgs memory marketsArgs,
-        uint256 initialDeposit
+        Vault.MarketsArgs memory marketsArgs
     )
         external
         containsCaller(salt)
@@ -91,14 +85,6 @@ contract VaultFactory {
             baseAsset, feeRecipient, feePercentage, name, symbol, initialDelay, initialDefaultAdmin, salt, marketsArgs
         );
 
-        baseAsset.safeTransferFrom(msg.sender, address(this), initialDeposit);
-        baseAsset.approve(address(vault), initialDeposit);
-        uint256 sharesMinted = vault.deposit(initialDeposit, address(this));
-
-        // The factory keeps 1e3 shares to reduce inflation attack vector.
-        // Effectively burns this amount of shares by locking it in the factory.
-        vault.transfer(msg.sender, sharesMinted - 1e3);
-
         emit CreateVault(address(vault), baseAsset, feeRecipient, feePercentage, name, symbol, initialDefaultAdmin);
     }
 }

test/fork/concrete/lrt/SpotOracle.t.sol

@@ -5,11 +5,11 @@ pragma solidity 0.8.21;
 import { ReserveOracle } from "../../../../src/oracles/reserve/ReserveOracle.sol";
 import { SpotOracle } from "../../../../src/oracles/spot/SpotOracle.sol";
 import { RsEthWstEthReserveOracle } from "../../../../src/oracles/reserve/lrt/RsEthWstEthReserveOracle.sol";
-import { RsEthWstEthSpotOracle } from "../../../../src/oracles/spot/lrt/rsEthWstEthSpotOracle.sol";
+import { RsEthWstEthSpotOracle } from "../../../../src/oracles/spot/lrt/RsEthWstEthSpotOracle.sol";
 import { RswEthWstEthReserveOracle } from "../../../../src/oracles/reserve/lrt/RswEthWstEthReserveOracle.sol";
-import { RswEthWstEthSpotOracle } from "../../../../src/oracles/spot/lrt/rswEthWstEthSpotOracle.sol";
+import { RswEthWstEthSpotOracle } from "../../../../src/oracles/spot/lrt/RswEthWstEthSpotOracle.sol";
 import { WeEthWstEthReserveOracle } from "../../../../src/oracles/reserve/lrt/WeEthWstEthReserveOracle.sol";
-import { WeEthWstEthSpotOracle } from "../../../../src/oracles/spot/lrt/weEthWstEthSpotOracle.sol";
+import { WeEthWstEthSpotOracle } from "../../../../src/oracles/spot/lrt/WeEthWstEthSpotOracle.sol";
 import { EzEthWstEthReserveOracle } from "./../../../../src/oracles/reserve/lrt/EzEthWstEthReserveOracle.sol";
 import { EzEthWstEthSpotOracle } from "./../../../../src/oracles/spot/lrt/EzEthWstEthSpotOracle.sol";
 import { EzEthWethReserveOracle } from "./../../../../src/oracles/reserve/lrt/EzEthWethReserveOracle.sol";