Stakewise - v3 Vaults / EthFoxVault

Description

Creating a Vault with an approved VaultFactory ensures that only approved implementations of Vaults can be registered with the system. For example, when a Vault Owner creates a new Vault via VaultFactory.createVault() the function deploys a proxy pointing to a fixed implementation. Additionally, the newly added Vault address is registered with the VaultRegistry. Only approved factories are allowed to register Vaults with the VaultRegistry.addVault().

However, the guarantee that only approved Factories (with approved implementations) can register Vaults is undermined by permissions the VaultRegistry owner has. They can unilaterally register Vaults that have not been created by the VaultFactory.

It is not clear if the registered Vault is actually a vault contract (or even EOA), nor if it’s been properly initialized within one transaction (see issue 4.5) as it can be registered out of band by the owner.

Examples

contracts/vaults/VaultsRegistry.sol:L31-L37

/// @inheritdoc IVaultsRegistry
function addVault(address vault) external override {
  if (!factories[msg.sender] && msg.sender != owner()) revert Errors.AccessDenied();

  vaults[vault] = true;
  emit VaultAdded(msg.sender, vault);
}

Recommendation

Ensure and provide guarantees on the origin of Vaults by enforcing that they’ve been created with an approved factory. If the owner is a multi-sig or DAO, ensure that everyone understands the implications of allowing the owner to add vaults to the registry out-of-band (SharedMevRewards) and the scrutiny required to avoid that a malicious Vault is added to the registry.

Description

• KeeperRewards.canUpdateRewards()

The inline comment mentions that the unchecked block cannot overflow as “lastRewardsTimestamp & rewardsDelay are uint64”. Yet, rewardsDelay is a uint256 meaning the result of lastRewardsTimestamp + rewardsDelay could overflow if rewardsDelay > 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF - lastRewardsTimestamp. If this operation resulted in an overflow, canUpdateRewards() would likely return true in invalid scenarios. This should not happen if the contract is initialized with reasonable values. However, in case the contract is incorrectly initialized or upgraded, this operation could overflow and lead to unintended effects.

contracts/keeper/KeeperRewards.sol:L132-L137

function canUpdateRewards() public view override returns (bool) {
  unchecked {
    // cannot overflow as lastRewardsTimestamp & rewardsDelay are uint64
    return lastRewardsTimestamp + rewardsDelay < block.timestamp;
  }
}

contracts/keeper/KeeperRewards.sol:L33

uint256 public immutable override rewardsDelay;

• VaultMev._harvestAssets()

An integer overflow may occur with an unsafe cast to int256(uint256) for values exceeding uint256.MAX/2+1. Although the likelihood of this happening within the current system and chain (ETH/Mainnet) is extremely low, requiring the contract to return an asset equivalent to over half of the ETH total supply, which is practically unfeasible. However, if this contract is reused, either wholly or partially, on different chains with different configurations or custom tokens, there could be a heightened risk. Therefore, to ensure robust security practices, it is advisable to invest a small amount of gas by implementing SafeCast to enforce secure coding standards.

contracts/vaults/modules/VaultMev.sol:L59-L63

  // execution rewards are always equal to what was accumulated in own MEV escrow
  return (totalAssetsDelta + int256(IOwnMevEscrow(_mevEscrow).harvest()), harvested);
}

contracts/vaults/ethereum/mev/OwnMevEscrow.sol:L23-L32

function harvest() external returns (uint256 assets) {
  if (msg.sender != vault) revert Errors.HarvestFailed();

  assets = address(this).balance;
  if (assets == 0) return 0;

  emit Harvested(assets);
  // slither-disable-next-line arbitrary-send-eth
  IVaultEthStaking(msg.sender).receiveFromMevEscrow{value: assets}();
}

Recommendation

For KeeperRewards.canUpdateRewards(), use a uint64 for rewardsDelay, remove the unchecked block or ensure rewardsDelay is within reasonable bounds to prevent any overflow. Ensure that comments are accurate. For VaultMev._harvestAssets() and other unsafe casts, use the SafeCast library to revert on over/underflows.

Description

VaultBlocklist and VaultWhitelist implement similar functionality. However, their handling from a management perspective is quite different.

• setBlocklistManager and setWhitelister implementations are the same. However, we would suggest using the same terminology for both functions (i.e. “BlocklistManager”, “WhitelistManager”).
• __VaultBlocklist_init takes a blocklistManager and stores it, while __VaultWhitelist_init also whitelists the whitelistManager. Given that a whitelister should only be used as a management account it is unclear why it needs to be whitelisted for e.g. token transfers?
• Blocklist exports a _checkBlocklist() method, while Whitelist requires callers to check the whitelistedAccounts mapping directly.
• updateBlocklist() silently proceeds even if the same address is blocklisted twice while _updateWhitelist() would revert

The differing management approaches between VaultBlocklist and VaultWhitelist introduce inconsistency and potential confusion for developers and users. This lack of uniformity in functionality and handling could lead to errors in implementation and management of blocklisted and whitelisted accounts, impacting the overall security and usability of the system.

Examples

• Blocklist updateBlocklist():

contracts/vaults/modules/VaultBlocklist.sol:L23-L29

function updateBlocklist(address account, bool isBlocked) public virtual override {
  if (msg.sender != blocklistManager) revert Errors.AccessDenied();
  if (blockedAccounts[account] == isBlocked) return;

  blockedAccounts[account] = isBlocked;
  emit BlocklistUpdated(msg.sender, account, isBlocked);
}

• Whitelist updateWhitelist():

contracts/vaults/modules/VaultWhitelist.sol:L23-L27

function updateWhitelist(address account, bool approved) external override {
  if (msg.sender != whitelister) revert Errors.AccessDenied();
  _updateWhitelist(account, approved);
}

contracts/vaults/modules/VaultWhitelist.sol:L34-L43

/**
 * @notice Internal function for updating whitelist
 * @param account The address of the account to update
 * @param approved Defines whether account is added to the whitelist or removed
 */
function _updateWhitelist(address account, bool approved) private {
  if (whitelistedAccounts[account] == approved) revert Errors.WhitelistAlreadyUpdated();
  whitelistedAccounts[account] = approved;
  emit WhitelistUpdated(msg.sender, account, approved);
}

Recommendation

Consider using the same or a similar interface for both Blocklist and Whitelist features.

Description

In the scenario where the Blocklist Manager intends to remove a user from the vault, particularly when the vault is not collateralized, the Blocklist Manager initiates the removal process by invoking the ejectUser() function within the EthFoxVault contract. Subsequently, this action triggers the execution of the redeem() path for the respective user.

contracts/vaults/ethereum/custom/EthFoxVault.sol:L87-L103

/// @inheritdoc IEthFoxVault
function ejectUser(address user) external override {
  // add user to blocklist
  updateBlocklist(user, true);

  // fetch shares of the user
  uint256 userShares = _balances[user];
  if (userShares == 0) return;

  if (_isCollateralized()) {
    // send user shares to exit queue
    _enterExitQueue(user, userShares, user);
  } else {
    // redeem user shares
    _redeem(user, userShares, user);
  }
}

contracts/vaults/modules/VaultEnterExit.sol:L164-L190

function _redeem(
  address user,
  uint256 shares,
  address receiver
) internal returns (uint256 assets) {
  _checkNotCollateralized();
  if (shares == 0) revert Errors.InvalidShares();
  if (receiver == address(0)) revert Errors.ZeroAddress();

  // calculate amount of assets to burn
  assets = convertToAssets(shares);
  if (assets == 0) revert Errors.InvalidAssets();

  // reverts in case there are not enough withdrawable assets
  if (assets > withdrawableAssets()) revert Errors.InsufficientAssets();

  // update total assets
  _totalAssets -= SafeCast.toUint128(assets);

  // burn owner shares
  _burnShares(user, shares);

  // transfer assets to the receiver
  _transferVaultAssets(receiver, assets);

  emit Redeemed(user, receiver, assets, shares);
}

This operation eventually leads to the execution of _transferVaultAssets(), a function responsible for facilitating a low-level value-contract-call to the designated recipient. In the event of an unsuccessful call, the operation reverts, ensuring the integrity of the transaction.

contracts/vaults/modules/VaultEthStaking.sol:L124-L130

/// @inheritdoc VaultEnterExit
function _transferVaultAssets(
  address receiver,
  uint256 assets
) internal virtual override nonReentrant {
  return Address.sendValue(payable(receiver), assets);
}

    function sendValue(address payable recipient, uint256 amount) internal {
        if (address(this).balance < amount) {
            revert AddressInsufficientBalance(address(this));
        }

        (bool success, ) = recipient.call{value: amount}("");
        if (!success) {
            revert FailedInnerCall();
        }
    }

Upon sending value to the designated recipient, the recipient’s fallback function, if available, is invoked, effectively transferring control to the recipient. At this juncture, the recipient possesses the option to revert the call within their fallback function. Should the recipient choose to revert the call, the outer call initiated at sendValue() would also revert accordingly.

In scenarios where the vault is not collateralized, the recipient can evade being ejected from the vault by intentionally reverting within their fallback function. This strategic maneuver allows the recipient to thwart the expulsion attempt initiated by the EthFoxVault contract during the redemption process.

Recommendation

Change from push to pull transfers. Document that a user can still be ejected by collateralizing the vault.

Description

Vault contracts like EthFoxVault are deployed as proxy contracts, which point to a specific implementation. The contract follows the OpenZeppelin (OZ) Initializable pattern without permissioning the initialize() function. This pattern necessitates that the deployment of the proxy contract and its initialization occur immediately, within the same transaction. Failure to do so exposes the contract to the risk of front-running, where any user could potentially manipulate the initialization process and claim administrative control over the contract.

contracts/vaults/ethereum/custom/EthFoxVault.sol:L49-L75

/// @custom:oz-upgrades-unsafe-allow constructor
constructor(
  address _keeper,
  address _vaultsRegistry,
  address _validatorsRegistry,
  address sharedMevEscrow,
  uint256 exitedAssetsClaimDelay
)
  VaultImmutables(_keeper, _vaultsRegistry, _validatorsRegistry)
  VaultEnterExit(exitedAssetsClaimDelay)
  VaultMev(sharedMevEscrow)
{
  _disableInitializers();
}

/// @inheritdoc IEthFoxVault
function initialize(bytes calldata params) external payable virtual override initializer {
  EthFoxVaultInitParams memory initParams = abi.decode(params, (EthFoxVaultInitParams));
  __EthFoxVault_init(initParams);
  emit EthFoxVaultCreated(
    initParams.admin,
    initParams.ownMevEscrow,
    initParams.capacity,
    initParams.feePercent,
    initParams.metadataIpfsHash
  );
}

Users deploying contracts via the VaultFactory are inherently protected, as both the deployment of the proxy and the initialization are executed within the same transaction.

contracts/vaults/ethereum/EthVaultFactory.sol:L38-L58

/// @inheritdoc IEthVaultFactory
function createVault(
  bytes calldata params,
  bool isOwnMevEscrow
) external payable override returns (address vault) {
  // create vault
  vault = address(new ERC1967Proxy(implementation, ''));

  // create MEV escrow contract if needed
  address _mevEscrow;
  if (isOwnMevEscrow) {
    _mevEscrow = address(new OwnMevEscrow(vault));
    // set MEV escrow contract so that it can be initialized in the Vault
    ownMevEscrow = _mevEscrow;
  }

  // set admin so that it can be initialized in the Vault
  vaultAdmin = msg.sender;

  // initialize Vault
  IEthVault(vault).initialize{value: msg.value}(params);

However, it’s worth noting that the task scripts located in the ./tasks/ directory do not utilize the VaultFactory, MultiCall, or Hardhat code to deploy and initialize in the same transaction. Consequently, vaults deployed via these task scripts are vulnerable to front-running by any party.

Examples

• FoxVault - vulnerable

tasks/eth-full-deploy.ts:L255-L278

const foxVault = foxVaultFactory.attach(foxVaultAddress)

// Initialize EthFoxVault
const ownMevEscrowFactory = await ethers.getContractFactory('OwnMevEscrow')
const ownMevEscrow = await ownMevEscrowFactory.deploy(foxVaultAddress)
await callContract(
  foxVault.initialize(
    ethers.AbiCoder.defaultAbiCoder().encode(
      [
        'tuple(address admin, address ownMevEscrow, uint256 capacity, uint16 feePercent, string metadataIpfsHash)',
      ],
      [
        [
          networkConfig.foxVault.admin,
          await ownMevEscrow.getAddress(),
          networkConfig.foxVault.capacity,
          networkConfig.foxVault.feePercent,
          networkConfig.foxVault.metadataIpfsHash,
        ],
      ]
    ),
    { value: networkConfig.securityDeposit }
  )
)

• Keeper, VaultsRegistry - not vulnerable due to function initialize() onlyOwner

tasks/eth-full-deploy.ts:L319-L324

// transfer ownership to governor
await callContract(vaultsRegistry.initialize(networkConfig.governor))
console.log('VaultsRegistry ownership transferred to', networkConfig.governor)

await callContract(keeper.initialize(networkConfig.governor))
console.log('Keeper ownership transferred to', networkConfig.governor)

• Test Suite does not even use Factory.createVault but deploy-initializes manually

test/shared/fixtures.ts:L715-L731

await vault.initialize(
  ethers.AbiCoder.defaultAbiCoder().encode(
    [
      'tuple(address admin, address ownMevEscrow, uint256 capacity, uint16 feePercent, string metadataIpfsHash)',
    ],
    [
      [
        adminAddr,
        await ownMevEscrow.getAddress(),
        vaultParams.capacity,
        vaultParams.feePercent,
        vaultParams.metadataIpfsHash,
      ],
    ]
  ),
  { value: SECURITY_DEPOSIT }
)

Recommendation

Note: According to the StakeWise team there is no intention to create a factory for the EthFoxVault. We recommend implementing safe deploy&initialize procedures instead of solely relying on verifying contract parameterization after deployment.

• Change the task scripts to use hardhat deploy & initialize code that performs all action in a single transaction.
• Switch to multicall deployment & initialize pattern.
• Enforce permission on initialize() to onlyOwner as seen with Keeper and VaultsRegistry
• Monitor and verify correct contract parameterisation after deployment

Description

The function ejectUser() in EthFoxVault allows the BlocklistManager to ban and eject a user from the system. Consider making this function emit a specific event for transparency and auditability reasons.

Examples

contracts/vaults/ethereum/custom/EthFoxVault.sol:L87-L88

/// @inheritdoc IEthFoxVault
function ejectUser(address user) external override {

Recommendation

Emit a specific event when a user is ejected from the system.

Description

Currently, the addVaultImpl() function in the VaultsRegistry contract lacks validation that address of newImpl actually has code. If an invalid address, devoid of code, is set as an implementation, vaults wishing to upgrade to that implementation will not be able to upgrade.

Example

• EthVaultFactory.implementation may not be a contract. This will cause a revert in createVault when ERC1967Proxy() is instantiated.

contracts/vaults/ethereum/EthVaultFactory.sol:L28-L36

/**
 * @dev Constructor
 * @param _implementation The implementation address of Vault
 * @param vaultsRegistry The address of the VaultsRegistry contract
 */
constructor(address _implementation, IVaultsRegistry vaultsRegistry) {
  implementation = _implementation;
  _vaultsRegistry = vaultsRegistry;
}

• VaultsRegistry.addVaultImpl does not validate newImpl. This will cause an implicit revert in UUPSUpgradeable._upgradeToAndCallUUPS during upgrade.

contracts/vaults/VaultsRegistry.sol:L40-L44

function addVaultImpl(address newImpl) external override onlyOwner {
  if (vaultImpls[newImpl]) revert Errors.AlreadyAdded();
  vaultImpls[newImpl] = true;
  emit VaultImplAdded(newImpl);
}

Recommendation

Enforce a check on newImpl.code.size > 0 early on.

Description

Consider using Solidity contract type interfaces for building low-level contract calls with arguments, instead of constructing them manually from function declaration strings.

Examples

contracts/vaults/modules/VaultVersion.sol:L26-L27

bytes4 private constant _initSelector = bytes4(keccak256('initialize(bytes)'));

contracts/vaults/modules/VaultVersion.sol:L34-L39

function upgradeToAndCall(
  address newImplementation,
  bytes memory data
) public payable override onlyProxy {
  super.upgradeToAndCall(newImplementation, abi.encodeWithSelector(_initSelector, data));
}

Recommendation

Use abi.encodeCall(IVault.initialize, (bytes data)) instead of falling back to low-level function selector calculations.

Description

The function _setWhitelister() (resp. _setBlocklistManager()) does not revert if the admin attempts to set the same _whitelister (resp. _blocklistManager ). This behavior might hide mistakes. Additionally, the functions emits a WhitelisterUpdated (resp. BlocklistManagerUpdated) event even though the _whitelister (resp. _blocklistManager ) hasn’t been modified.

Examples

• Blocklist

contracts/vaults/modules/VaultBlocklist.sol:L31-L35

/// @inheritdoc IVaultBlocklist
function setBlocklistManager(address _blocklistManager) external override {
  _checkAdmin();
  _setBlocklistManager(_blocklistManager);
}

contracts/vaults/modules/VaultBlocklist.sol:L49-L53

function _setBlocklistManager(address _blocklistManager) private {
  // update blocklist manager address
  blocklistManager = _blocklistManager;
  emit BlocklistManagerUpdated(msg.sender, _blocklistManager);
}

• Whitelist

contracts/vaults/modules/VaultWhitelist.sol:L28-L32

/// @inheritdoc IVaultWhitelist
function setWhitelister(address _whitelister) external override {
  _checkAdmin();
  _setWhitelister(_whitelister);
}

contracts/vaults/modules/VaultWhitelist.sol:L45-L53

/**
 * @dev Internal function for updating the whitelister externally or from the initializer
 * @param _whitelister The address of the new whitelister
 */
function _setWhitelister(address _whitelister) private {
  // update whitelister address
  whitelister = _whitelister;
  emit WhitelisterUpdated(msg.sender, _whitelister);
}

Recommendation

Consider reverting the _setWhitelister() (resp. _setBlocklistManager()) function execution in case one attempts to set the same _whitelister (resp. _blocklistManager ).

Description

The Vault admin is set on initialization.

• There is no address(0) check preventing no admin from being set at initialization.
• Admin access can only be set on initialization and not be transferred (2-step). This may leave the Vault vulnerable and the admins unable to react in case of a vault admin address compromise.

contracts/vaults/modules/VaultAdmin.sol:L24-L34

/**
 * @dev Initializes the VaultAdmin contract
 * @param _admin The address of the Vault admin
 */
function __VaultAdmin_init(
  address _admin,
  string memory metadataIpfsHash
) internal onlyInitializing {
  admin = _admin;
  emit MetadataUpdated(msg.sender, metadataIpfsHash);
}

Example

For reference, Fee recipient invalidates address(0)

contracts/vaults/modules/VaultFee.sol:L36-L39

function _setFeeRecipient(address _feeRecipient) private {
  _checkHarvested();
  if (_feeRecipient == address(0)) revert Errors.InvalidFeeRecipient();

Recommendation

Consider implementing a 2-step vault admin transfer and checking that the admin is not address(0) to prevent any mistake.

Description

Declare state variables with the best type available and downcast to address if needed. Typecasting inside the corpus of a function is unneeded when the parameter’s type is known beforehand. Declare the best type in function arguments and state variables. Always return the best type available instead of resorting to address by default.

Examples

There are more instances of this pattern, but here’s a list of samples:

contracts/vaults/ethereum/EthGenesisVault.sol:L71-L75

{
  _poolEscrow = IPoolEscrow(poolEscrow);
  _rewardEthToken = IRewardEthToken(rewardEthToken);
}

contracts/vaults/ethereum/EthGenesisVault.sol:L51-L61

constructor(
  address _keeper,
  address _vaultsRegistry,
  address _validatorsRegistry,
  address osTokenVaultController,
  address osTokenConfig,
  address sharedMevEscrow,
  address poolEscrow,
  address rewardEthToken,
  uint256 exitingAssetsClaimDelay
)

contracts/vaults/modules/VaultImmutables.sol:L13-L22

abstract contract VaultImmutables {
  /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
  address internal immutable _keeper;

  /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
  address internal immutable _vaultsRegistry;

  /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
  address internal immutable _validatorsRegistry;

contracts/vaults/modules/VaultVersion.sol:L41-L53

/// @inheritdoc UUPSUpgradeable
function _authorizeUpgrade(address newImplementation) internal view override {
  _checkAdmin();
  if (
    newImplementation == address(0) ||
    ERC1967Utils.getImplementation() == newImplementation || // cannot reinit the same implementation
    IVaultVersion(newImplementation).vaultId() != vaultId() || // vault must be of the same type
    IVaultVersion(newImplementation).version() != version() + 1 || // vault cannot skip versions between
    !IVaultsRegistry(_vaultsRegistry).vaultImpls(newImplementation) // new implementation must be registered
  ) {
    revert Errors.UpgradeFailed();
  }
}

Description

To mitigate the withdrawal credentials front-running vulnerability, Stakewise requires oracles to sign the validators registry’s Merkle tree root when approving new validators. This ensures that if a malicious operator attempts to front-run a legitimate deposit transaction with different withdrawal credentials, the Merkle tree root of the deposit contract will change, invalidating the legitimate deposit transaction through a check in the KeeperValidators contract:

contracts/keeper/KeeperValidators.sol:L53-L55

if (_validatorsRegistry.get_deposit_root() != params.validatorsRegistryRoot) {
  revert Errors.InvalidValidatorsRegistryRoot();
}

It should be noted that this mechanism potentially opens the door to a DoS attack. Specifically, a malicious actor could theoretically disrupt validator approval by front-running legitimate deposit transactions with a deposit of at least 1 ETH into the validator registry contract. However, such an attack would likely be costly and resource-intensive to sustain over time.

Description

Follow the solidity style guide. Specifically, constants should be named with all capital letters with underscores separating words. Examples: MAX_BLOCKS, TOKEN_NAME, TOKEN_TICKER, CONTRACT_VERSION.

Examples

contracts/vaults/modules/VaultEthStaking.sol:L31

uint256 private constant _securityDeposit = 1e9;

contracts/vaults/modules/VaultFee.sol:L18-L19

uint256 internal constant _maxFeePercent = 10_000; // @dev 100.00 %

contracts/vaults/modules/VaultValidators.sol:L26-L27

uint256 internal constant _validatorLength = 176;

contracts/vaults/modules/VaultVersion.sol:L26-L27

bytes4 private constant _initSelector = bytes4(keccak256('initialize(bytes)'));

Description

Consider taking the if-branch for totalAssetsDelta less than or equal zero and return early instead of consuming gas on mulDiv and performing accounting.

contracts/vaults/modules/VaultState.sol:L101-L116

/**
 * @dev Internal function for processing rewards and penalties
 * @param totalAssetsDelta The number of assets earned or lost
 */
function _processTotalAssetsDelta(int256 totalAssetsDelta) internal {
  // SLOAD to memory
  uint256 newTotalAssets = _totalAssets;
  if (totalAssetsDelta < 0) {
    // add penalty to total assets
    newTotalAssets -= uint256(-totalAssetsDelta);

    // update state
    _totalAssets = SafeCast.toUint128(newTotalAssets);
    return;
  }

Recommendation

The logical error can be addressed by modifying the conditional check to if (totalAssetsDelta <= 0). This accommodates when the totalAssetsDelta parameter is 0. When it is so, neither a fee should be deducted from nor should the function proceed with the reward processing. This if-statement can simply return in such a case.

 function _processTotalAssetsDelta(int256 totalAssetsDelta) internal {
   // SLOAD to memory 
   uint256 newTotalAssets = _totalAssets;
   if (totalAssetsDelta =< 0) { 
     // ...

Description

Several source units contain imports for libraries or contracts that are not utilized within the codebase.

Unused imports contribute to code clutter and may confuse developers about the library’s use in the contract. Keeping the codebase clean can help with maintainability and readability.

Example

• Unused Import

contracts/vaults/modules/VaultEnterExit.sol:L8

import {IKeeperRewards} from '../../interfaces/IKeeperRewards.sol';

• Unused Import

contracts/vaults/ethereum/EthPrivVault.sol:L14

import {VaultVersion} from '../modules/VaultVersion.sol';

• Duplicate Import

contracts/vaults/ethereum/EthErc20Vault.sol:L8-L10

import {IEthVaultFactory} from '../../interfaces/IEthVaultFactory.sol';
import {IEthErc20Vault} from '../../interfaces/IEthErc20Vault.sol';
import {IEthVaultFactory} from '../../interfaces/IEthVaultFactory.sol';

Recommendation

Adhering to best practices, it is recommended to eliminate any unused imports to ensure the cleanliness of the codebase. Consequently, the removal of these unused imports from the contracts is advisable to maintain codebase integrity and enhance overall code quality.