hats-finance / Velvet-Capital-0x0bb0c08fd9eeaf190064f4c66f11d18182961f77

Core smart contracts of Velvet Capital
Other
0 stars 1 forks source link

Vault can never be initialised if `tokenWhitelistingEnabled = false` #71

Open hats-bug-reporter[bot] opened 6 days ago

hats-bug-reporter[bot] commented 6 days ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xaf4fc671e63c54d01fb3c3546fdab6721b288d602a0464bd7bc4544a7a4dd746 Severity: high

Description: Description\ The VaultConfig.sol contract is used to initialise a vault with set of tokens, however if the owner choose to disable whitelisting feature the vault can never be initialised.

Attack Scenario\ Let's take a look at how a vault is initialised :

 function initToken(address[] calldata _tokens) external onlySuperAdmin {
    uint256 _assetLimit = protocolConfig().assetLimit();
    uint256 tokensLength = _tokens.length;
    if (tokensLength > _assetLimit)
      revert ErrorLibrary.TokenCountOutOfLimit(_assetLimit);
    if (tokens.length != 0) {
      revert ErrorLibrary.AlreadyInitialized();
    }
    for (uint256 i; i < tokensLength; i++) {
      address token = _tokens[i];
      _beforeInitCheck(token);//@audit vaults will not be initialised if one of the tokens is not whitelisted or if the token whitelisting flag is disabled
      if (_previousToken[token]) {
        revert ErrorLibrary.TokenAlreadyExist();
      }
      _previousToken[token] = true;
      tokens.push(token);
    }
    _resetPreviousTokenList(_tokens);
    emit PublicSwapEnabled(address(this));
  }

The above function is used Initializes the vault with a set of _tokens and it has a check for every token within the for Loop _beforeInitCheck(token); which makes sure the tokens are whitelisted, however if you look at the code of _beforeInitCheck(token); check:

 function _beforeInitCheck(address token) internal {
        if ((assetManagementConfig().tokenWhitelistingEnabled() && !assetManagementConfig().whitelistedTokens(token))) {
            revert ErrorLibrary.TokenNotWhitelisted();
        }
        if (token == address(0)) {
            revert ErrorLibrary.InvalidTokenAddress();
        }
    }

The above function has a check

 if ((assetManagementConfig().tokenWhitelistingEnabled() && !assetManagementConfig().whitelistedTokens(token))) {
            revert ErrorLibrary.TokenNotWhitelisted();
        }

Now this checks if the token whitelisting is enabled or disabled , and if token whitelisting is disabled this whole transaction will revert and the vault will not be initialised in that case! Vaults will also be left uninitialised if one of the tokens is not whitelisted however this seems intended but the previous case where the bool public tokenWhitelistingEnabled; is false will definitely screw up the whole initialisation process , because there is an option for disabling token whitelisting feature it is for sure that this can be choosen as an option and in that case the vault can never be initialised due to revert!

Attachments

  1. Proof of Concept (PoC) File

  2. Revised Code File (Optional) Refactor the code according to it

langnavina97 commented 6 days ago

The transaction will not revert if tokenWhitelisting is set to false. The condition includes an "&&" check, so if one condition is false, it won't check further. If there is no token whitelisting, "assetManagementConfig().tokenWhitelistingEnabled()" will result in false.

0xWeb3boy commented 6 days ago

@langnavina97 I think I misunderstood and probably submitted this in haste! However there is one more important aspect in this submission 'Vaults will also be left uninitialised if one of the tokens is not whitelisted' since there is an array for tokens initialisation and if one of the tokens is not whitelisted the transaction will revert and the vault will not be initialised in that case!

langnavina97 commented 6 days ago

This was already mentioned #33 @0xWeb3boy