hats-finance / StakeWise-0xd91cd6ed6c9a112fdc112b1a3c66e47697f522cd

Liquid staking protocol for Ethereum
Other
0 stars 0 forks source link

Shares can be transferred to un-whitelisted accounts in `EthPrivErc20Vault.sol` #39

Open hats-bug-reporter[bot] opened 1 year ago

hats-bug-reporter[bot] commented 1 year ago

Github username: @milotruck Submission hash (on-chain): 0x8d2352dc289982ddbdc1e8054321b700ae83cfdb387dd32cd47a848bc8b14d2e Severity: medium

Description:

Bug Description

In EthPrivErc20Vault.sol, the vault admin has full control over which addresses can deposit/withdraw ETH from the vault. This can be seen in the documentation:

Whitelist is a function in Private Vaults that allows Vault Admin to control who can deposit and withdraw ETH from the Vault.

This is enforced using a whitelist, as seen in deposit():

EthPrivErc20Vault.sol#L78-L86

  function deposit(
    address receiver,
    address referrer
  ) public payable virtual override(IVaultEthStaking, VaultEthStaking) returns (uint256 shares) {
    if (!(whitelistedAccounts[msg.sender] && whitelistedAccounts[receiver])) {
      revert Errors.AccessDenied();
    }
    return super.deposit(receiver, referrer);
  }

As seen from above, the function checks that receiver is whitelisted in whitelistedAccounts. This allows the vault's admin to have full control over which address holds shares using the whitelist.

However, the transfer() and transferFrom() functions do not validate that the receiving address is whitelisted. This allows users to bypass the whitelist and transfer shares to un-whitelisted addresses, for example:

In this scenario, although Bob is not whitelisted by the vault's admin, he now holds shares and can earn yield from the vault, and can call the vault's other functions, such as redeem().

Impact

In EthPrivErc20Vault, due to the lack of access control in transfer() and transferFrom(), users can transfer shares to un-whitelisted addresses. As a result, the vault's admin will have no control over which addesses can earn yield/withdraw ETH from the vault, contradicting what is stated in the documentation.

Recommended Mitigation

In EthPrivErc20Vault.sol, consider overriding the transfer() and transferFrom() functions to check if the to address is whitelisted:

function transfer(
  address to,
  uint256 amount
) public virtual override returns (bool) {
  if (!whitelistedAccounts[to]) revert Errors.AccessDenied();
  return super.transfer(to, amount);
}
function transferFrom(
  address from,
  address to,
  uint256 amount
) public virtual override returns (bool) {
  if (!whitelistedAccounts[to]) revert Errors.AccessDenied();
  return super.transferFrom(from, to, amount);
}
tsudmi commented 1 year ago

We thought to add that initially, but decided to follow the USDC approach, where only KYCed/whitelisted users can deposit USD and mint USDC, but anyone can transfer. The whitelisting is meant to check who deposits ETH to the vault, as you may not want to receive it from not KYCed users.