Merge pull request #87 from credbull/audit-v2-vault-remediation

Audit v2 vault remediation

packages/contracts/CBL_README.md

@@ -112,3 +112,9 @@ error CBL__InvalidOwnerAddress();
 error CBL__InvalidMinterAddress();
 ```
 
+## Roadmap - Future Features
+
+### Governance & Governance Token ($gCBL) Contract
+
+Governance will be implemented in a separate Governance Token ($gCBL) smart contract.   The $gCBL contract will include functions 
+related to governance and voting.  Users lock $CBL to receive Governance Tokens ($gCBL) in return.

packages/contracts/src/factory/VaultFactory.sol

@@ -14,6 +14,21 @@ abstract contract VaultFactory is AccessControl {
     /// @notice Error to revert if custodian is not allowed
     error CredbullVaultFactory__CustodianNotAllowed();
 
+    /// @notice Error to indicate that the provided owner address is invalid.
+    error CredbullVaultFactory__InvalidOwnerAddress();
+
+    /// @notice Error to indicate that the provided operator address is invalid.
+    error CredbullVaultFactory__InvalidOperatorAddress();
+
+    /// @notice Error to indicate that the provided custodian address is invalid.
+    error CredbullVaultFactory__InvalidCustodianAddress();
+
+    /// @notice Event to emit when a new custodian is allowed
+    event CustodianAllowed(address indexed custodian);
+
+    /// @notice Event to emit when a custodian is removed
+    event CustodianRemoved(address indexed custodian);
+
     /// @notice Address set that contains list of all vault address
     EnumerableSet.AddressSet internal allVaults;
 
@@ -29,12 +44,22 @@ abstract contract VaultFactory is AccessControl {
      * @param custodians - Initial set of custodians allowable for the vaults
      */
     constructor(address owner, address operator, address[] memory custodians) {
+        if (owner == address(0)) {
+            revert CredbullVaultFactory__InvalidOwnerAddress();
+        }
+
+        if (operator == address(0)) {
+            revert CredbullVaultFactory__InvalidOperatorAddress();
+        }
         _grantRole(DEFAULT_ADMIN_ROLE, owner);
         _grantRole(OPERATOR_ROLE, operator);
 
         // set the allowed custodians directly in the constructor, without access restriction
         bool[] memory result = new bool[](custodians.length);
         for (uint256 i = 0; i < custodians.length; i++) {
+            if (custodians[i] == address(0)) {
+                revert CredbullVaultFactory__InvalidCustodianAddress();
+            }
             result[i] = allowedCustodians.add(custodians[i]);
         }
     }
@@ -58,13 +83,19 @@ abstract contract VaultFactory is AccessControl {
 
     /// @notice Add custodian address to the set
     function allowCustodian(address _custodian) public onlyRole(DEFAULT_ADMIN_ROLE) returns (bool) {
+        if (_custodian == address(0)) {
+            revert CredbullVaultFactory__InvalidCustodianAddress();
+        }
+
+        emit CustodianAllowed(_custodian);
         return allowedCustodians.add(_custodian);
     }
 
     /// @notice Remove custodian address from the set
     function removeCustodian(address _custodian) public onlyRole(DEFAULT_ADMIN_ROLE) {
         if (allowedCustodians.contains(_custodian)) {
             allowedCustodians.remove(_custodian);
+            emit CustodianRemoved(_custodian);
         }
     }
 

packages/contracts/src/plugin/MaxCapPlugin.sol

@@ -6,6 +6,12 @@ pragma solidity ^0.8.20;
 abstract contract MaxCapPlugin {
     error CredbullVault__MaxCapReached();
 
+    /// @notice Event emitted when the max cap is updated
+    event MaxCapUpdated(uint256 indexed maxCap);
+
+    /// @notice Event emitted when the max cap check is updated
+    event MaxCapCheckUpdated(bool indexed checkMaxCap);
+
     /// @notice - Params for the MaxCap Plugin
     struct MaxCapPluginParams {
         uint256 maxCap;
@@ -33,12 +39,16 @@ abstract contract MaxCapPlugin {
     }
 
     /// @notice - Toggle the max cap check status
-    function _toggleMaxCapCheck(bool status) internal virtual {
-        checkMaxCap = status;
+    function _setCheckMaxCap(bool _checkMaxCapStatus) internal virtual {
+        checkMaxCap = _checkMaxCapStatus;
+
+        emit MaxCapCheckUpdated(checkMaxCap);
     }
 
     /// @notice - Update the max cap value
     function _updateMaxCap(uint256 _value) internal virtual {
         maxCap = _value;
+
+        emit MaxCapUpdated(maxCap);
     }
 }

packages/contracts/src/plugin/WhiteListPlugin.sol

@@ -11,40 +11,44 @@ abstract contract WhiteListPlugin {
     /// @notice Error to revert if the address is not whiteListed
     error CredbullVault__NotWhiteListed(address, uint256);
 
+    /// @notice Event emitted when the whiteList check is updated
+    event WhiteListCheckUpdated(bool indexed checkWhiteList);
+
     /// @notice - Params for the WhiteList Plugin
     struct WhiteListPluginParams {
         address whiteListProvider;
         uint256 depositThresholdForWhiteListing;
     }
 
     /// @notice - Address of the White List Provider.
-    IWhiteListProvider public whiteListProvider;
+    IWhiteListProvider public immutable WHITELIST_PROVIDER;
 
     /// @notice - Flag to check for whiteList
     bool public checkWhiteList;
 
     /// @notice - Deposit threshold amount to check for whiteListing
-    uint256 public depositThresholdForWhiteListing;
+    uint256 public immutable DEPOSIT_THRESHOLD_FOR_WHITE_LISTING;
 
     constructor(WhiteListPluginParams memory params) {
         if (params.whiteListProvider == address(0)) {
             revert CredbullVault__InvalidWhiteListProviderAddress(params.whiteListProvider);
         }
 
-        whiteListProvider = IWhiteListProvider(params.whiteListProvider);
+        WHITELIST_PROVIDER = IWhiteListProvider(params.whiteListProvider);
         checkWhiteList = true; // Set the check to true by default
-        depositThresholdForWhiteListing = params.depositThresholdForWhiteListing;
+        DEPOSIT_THRESHOLD_FOR_WHITE_LISTING = params.depositThresholdForWhiteListing;
     }
 
     /// @notice - Function to check for whiteListed address
     function _checkIsWhiteListed(address receiver, uint256 amount) internal view virtual {
-        if (checkWhiteList && amount >= depositThresholdForWhiteListing && !whiteListProvider.status(receiver)) {
+        if (checkWhiteList && amount >= DEPOSIT_THRESHOLD_FOR_WHITE_LISTING && !WHITELIST_PROVIDER.status(receiver)) {
             revert CredbullVault__NotWhiteListed(receiver, amount);
         }
     }
 
     /// @notice - Function to toggle check for whiteListed address
-    function _toggleWhiteListCheck(bool status) internal virtual {
-        checkWhiteList = status;
+    function _toggleWhiteListCheck() internal virtual {
+        checkWhiteList = !checkWhiteList;
+        emit WhiteListCheckUpdated(checkWhiteList);
     }
 }

packages/contracts/src/plugin/WindowPlugin.sol

@@ -9,6 +9,26 @@ abstract contract WindowPlugin {
         uint256 windowOpensAt, uint256 windowClosesAt, uint256 timestamp
     );
 
+    /// @notice Error to revert when incorrect window values are provided
+    error WindowPlugin__IncorrectWindowValues(
+        uint256 depositOpen, uint256 depositClose, uint256 redeemOpen, uint256 redeemClose
+    );
+
+    /// @notice Event emitted when the window is updated
+    event WindowUpdated(
+        uint256 depositOpensAt, uint256 depositClosesAt, uint256 redemptionOpensAt, uint256 redemptionClosesAt
+    );
+
+    /// @notice Event emitted when the window check is updated
+    event WindowCheckUpdated(bool indexed checkWindow);
+
+    modifier validateWindows(uint256 _depositOpen, uint256 _depositClose, uint256 _redeemOpen, uint256 _redeemClose) {
+        if (!(_depositOpen < _depositClose && _depositClose < _redeemOpen && _redeemOpen < _redeemClose)) {
+            revert WindowPlugin__IncorrectWindowValues(_depositOpen, _depositClose, _redeemOpen, _redeemClose);
+        }
+        _;
+    }
+
     /// @notice A Window is essentially a Time Span, denoted by an Opening and Closing Time pair.
     struct Window {
         uint256 opensAt;
@@ -36,7 +56,14 @@ abstract contract WindowPlugin {
     /// @notice - Flag to check for window
     bool public checkWindow;
 
-    constructor(WindowPluginParams memory params) {
+    constructor(WindowPluginParams memory params)
+        validateWindows(
+            params.depositWindow.opensAt,
+            params.depositWindow.closesAt,
+            params.redemptionWindow.opensAt,
+            params.redemptionWindow.closesAt
+        )
+    {
         depositOpensAtTimestamp = params.depositWindow.opensAt;
         depositClosesAtTimestamp = params.depositWindow.closesAt;
         redemptionOpensAtTimestamp = params.redemptionWindow.opensAt;
@@ -68,15 +95,19 @@ abstract contract WindowPlugin {
     function _updateWindow(uint256 _depositOpen, uint256 _depositClose, uint256 _redeemOpen, uint256 _redeemClose)
         internal
         virtual
+        validateWindows(_depositOpen, _depositClose, _redeemOpen, _redeemClose)
     {
         depositOpensAtTimestamp = _depositOpen;
         depositClosesAtTimestamp = _depositClose;
         redemptionOpensAtTimestamp = _redeemOpen;
         redemptionClosesAtTimestamp = _redeemClose;
+
+        emit WindowUpdated(_depositOpen, _depositClose, _redeemOpen, _redeemClose);
     }
 
     /// @notice - Function to toggle check for window
-    function _toggleWindowCheck(bool status) internal {
-        checkWindow = status;
+    function _toggleWindowCheck() internal {
+        checkWindow = !checkWindow;
+        emit WindowCheckUpdated(checkWindow);
     }
 }

packages/contracts/src/provider/whiteList/WhiteListProvider.sol

@@ -3,8 +3,9 @@ pragma solidity ^0.8.20;
 
 import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol";
 import { IWhiteListProvider } from "./IWhiteListProvider.sol";
+import { Ownable2Step } from "@openzeppelin/contracts/access/Ownable2Step.sol";
 
-contract WhiteListProvider is IWhiteListProvider, Ownable {
+contract WhiteListProvider is IWhiteListProvider, Ownable2Step {
     error LengthMismatch();
 
     /**
@@ -30,6 +31,7 @@ contract WhiteListProvider is IWhiteListProvider, Ownable {
         uint256 length = _addresses.length;
 
         for (uint256 i; i < length;) {
+            if (_addresses[i] == address(0)) continue;
             isWhiteListed[_addresses[i]] = _statuses[i];
 
             unchecked {

packages/contracts/src/vault/FixedYieldVault.sol

@@ -13,6 +13,12 @@ import { MaxCapPlugin } from "../plugin/MaxCapPlugin.sol";
 contract FixedYieldVault is MaturityVault, WhiteListPlugin, WindowPlugin, MaxCapPlugin, AccessControl {
     using Math for uint256;
 
+    /// @notice Error to indicate that the provided owner address is invalid.
+    error FixedYieldVault__InvalidOwnerAddress();
+
+    /// @notice Error to indicate that the provided operator address is invalid.
+    error FixedYieldVault__InvalidOperatorAddress();
+
     /// @notice - Hash of operator role
     bytes32 public constant OPERATOR_ROLE = keccak256("OPERATOR_ROLE");
 
@@ -32,18 +38,26 @@ contract FixedYieldVault is MaturityVault, WhiteListPlugin, WindowPlugin, MaxCap
     }
 
     /// @dev The fixed yield value in percentage(100) that's promised to the users on deposit.
-    uint256 private _fixedYield;
+    uint256 private immutable FIXED_YIELD;
 
     constructor(FixedYieldVaultParams memory params)
         MaturityVault(params.maturityVault)
         WhiteListPlugin(params.whiteListPlugin)
         WindowPlugin(params.windowPlugin)
         MaxCapPlugin(params.maxCapPlugin)
     {
+        if (params.roles.owner == address(0)) {
+            revert FixedYieldVault__InvalidOwnerAddress();
+        }
+
+        if (params.roles.operator == address(0)) {
+            revert FixedYieldVault__InvalidOperatorAddress();
+        }
+
         _grantRole(DEFAULT_ADMIN_ROLE, params.roles.owner);
         _grantRole(OPERATOR_ROLE, params.roles.operator);
 
-        _fixedYield = params.promisedYield;
+        FIXED_YIELD = params.promisedYield;
     }
 
     /// @dev - Overridden deposit modifer
@@ -70,7 +84,7 @@ contract FixedYieldVault is MaturityVault, WhiteListPlugin, WindowPlugin, MaxCap
 
     // @notice - Returns expected assets on maturity
     function expectedAssetsOnMaturity() public view override returns (uint256) {
-        return totalAssetDeposited.mulDiv(100 + _fixedYield, 100);
+        return totalAssetDeposited.mulDiv(100 + FIXED_YIELD, 100);
     }
 
     /// @notice Mature the vault
@@ -79,23 +93,23 @@ contract FixedYieldVault is MaturityVault, WhiteListPlugin, WindowPlugin, MaxCap
     }
 
     /// @notice Toggle check for maturity
-    function toggleMaturityCheck(bool status) public onlyRole(DEFAULT_ADMIN_ROLE) {
-        _toggleMaturityCheck(status);
+    function setMaturityCheck(bool _setMaturityCheckStatus) public onlyRole(DEFAULT_ADMIN_ROLE) {
+        _setMaturityCheck(_setMaturityCheckStatus);
     }
 
     /// @notice Toggle check for whiteList
-    function toggleWhiteListCheck(bool status) public onlyRole(DEFAULT_ADMIN_ROLE) {
-        _toggleWhiteListCheck(status);
+    function toggleWhiteListCheck() public onlyRole(DEFAULT_ADMIN_ROLE) {
+        _toggleWhiteListCheck();
     }
 
     /// @notice Toggle check for window
-    function toggleWindowCheck(bool status) public onlyRole(DEFAULT_ADMIN_ROLE) {
-        _toggleWindowCheck(status);
+    function toggleWindowCheck() public onlyRole(DEFAULT_ADMIN_ROLE) {
+        _toggleWindowCheck();
     }
 
     /// @notice Toggle check for max cap
-    function toggleMaxCapCheck(bool status) public onlyRole(DEFAULT_ADMIN_ROLE) {
-        _toggleMaxCapCheck(status);
+    function setCheckMaxCap(bool _checkMaxCapStatus) public onlyRole(DEFAULT_ADMIN_ROLE) {
+        _setCheckMaxCap(_checkMaxCapStatus);
     }
 
     /// @notice Update max cap value

packages/contracts/src/vault/MaturityVault.sol

@@ -23,6 +23,12 @@ abstract contract MaturityVault is Vault {
     /// @notice Reverts on mature if there is not enough balance.
     error CredbullVault__NotEnoughBalanceToMature();
 
+    /// @notice Event emitted when the vault matures.
+    event VaultMatured(uint256 indexed totalAssetDeposited);
+
+    /// @notice Event emitted when the maturity check is updated.
+    event MaturityCheckUpdated(bool indexed checkMaturity);
+
     /// @notice Determine if the vault is matured or not.
     bool public isMatured;
 
@@ -48,6 +54,8 @@ abstract contract MaturityVault is Vault {
 
         totalAssetDeposited = currentBalance;
         isMatured = true;
+
+        emit VaultMatured(totalAssetDeposited);
     }
 
     /// @notice - Returns expected assets on maturity
@@ -73,9 +81,10 @@ abstract contract MaturityVault is Vault {
     /**
      * @notice Enables/disables the Maturity Check according to the [status] value.
      * @dev 'Toggling' means flipping the existing state. This is simply a mutator.
-     * @param status Boolean value to toggle
      */
-    function _toggleMaturityCheck(bool status) internal {
-        checkMaturity = status;
+    function _setMaturityCheck(bool _setMaturityCheckStatus) internal {
+        checkMaturity = _setMaturityCheckStatus;
+
+        emit MaturityCheckUpdated(checkMaturity);
     }
 }