Attacker can steal shares of "vault-like" tokens, such as stETH, that should belong to legitimate vaults

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L131-L140 https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L143-L160 https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L185-L207 https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L209-L238

Vulnerability details

Impact

An attacker can call the following depositWithExpiry function with an address controlled by him as the vault input. After the _deposit function below is called, he has sent the amount tokens to the router, and _vaultAllowance[vault][asset] for his controlled address has increased by the amount. This is problematic when the asset is a "vault-like" token, such as stETH that needs to be supported by this protocol according to this protocol's token whitelist.

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L131-L140

  function depositWithExpiry(
    address payable vault,
    address asset,
    uint amount,
    string memory memo,
    uint expiration
  ) external payable {
    require(block.timestamp < expiration, "THORChain_Router: expired");
    _deposit(vault, asset, amount, memo);
  }

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L143-L160

  function _deposit(
    address payable vault,
    address asset,
    uint amount,
    string memory memo
  ) private nonReentrant {
    uint safeAmount;
    if (asset == address(0)) {
      ...
    } else {
      require(msg.value == 0, "unexpected eth"); // protect user from accidentally locking up eth
      safeAmount = safeTransferFrom(asset, amount); // Transfer asset
      _vaultAllowance[vault][asset] += safeAmount; // Credit to chosen vault
    }
    ...
  }

As shown by the StETH contract's functions below, calling the StETH.transferFrom function, which eventually calls the StETH.getSharesByPooledEth function, updates the stETH holders' shares instead of updating their balances directly. The stETH holders' shares are stored in the StETH contract, and according to the StETH.balanceOf function, their stETH balances corresponding to their shares are dynamic depending on the total pooled Ether and total shares for the StETH contract. After the attacker's depositWithExpiry function call, the router's shares is increased by a share amount that corresponds to the amount stETH tokens based on the total pooled Ether and total shares for the StETH contract at the moment of the attacker's deposit. Subsequently, more deposits can occur for legitimate vaults, and the router's stETH shares are further increased.

https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code#F27#L237

    function transferFrom(address _sender, address _recipient, uint256 _amount) external returns (bool) {
        _spendAllowance(_sender, msg.sender, _amount);
        _transfer(_sender, _recipient, _amount);
        return true;
    }

https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code#F27#L375

    function _transfer(address _sender, address _recipient, uint256 _amount) internal {
        uint256 _sharesToTransfer = getSharesByPooledEth(_amount);
        _transferShares(_sender, _recipient, _sharesToTransfer);
        ...
    }

https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code#F27#L300

    function getSharesByPooledEth(uint256 _ethAmount) public view returns (uint256) {
        return _ethAmount
            .mul(_getTotalShares())
            .div(_getTotalPooledEther());
    }

https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code#F27#L441

    function _transferShares(address _sender, address _recipient, uint256 _sharesAmount) internal {
        ...
        uint256 currentSenderShares = shares[_sender];
        ...
        shares[_sender] = currentSenderShares.sub(_sharesAmount);
        shares[_recipient] = shares[_recipient].add(_sharesAmount);
    }

https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code#F27#L166

    function balanceOf(address _account) external view returns (uint256) {
        return getPooledEthByShares(_sharesOf(_account));
    }

https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code#F27#L309

    function getPooledEthByShares(uint256 _sharesAmount) public view returns (uint256) {
        return _sharesAmount
            .mul(_getTotalPooledEther())
            .div(_getTotalShares());
    }

When more on chain interactions with the StETH contract happen, the total pooled Ether and total shares for the StETH contract are changed; after such change, based on the StETH.getSharesByPooledEth function, the amount stETH tokens can be equivalent to an stETH share amount that is higher than the router's stETH share increment for the attacker's deposit of the same amount stETH tokens. Then, the attacker can use his controlled address to call the following transferOut or _transferOutV5 function to send all of the _vaultAllowance for his controlled address, which equals the amount, to himself. As shown below, since StETH.transfer function also eventually calls the StETH.getSharesByPooledEth function, for the same amount stETH tokens, the attacker would receive an stETH share amount that is higher than what was removed from him for his deposit; in this case, the extra stETH shares sent to the attacker should belong to the legitimate vaults. As a result, the attacker has stolen some of the stETH shares that should be controlled by the legitimate vaults.

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L185-L207

  function transferOut(
    address payable to,
    address asset,
    uint amount,
    string memory memo
  ) public payable nonReentrant {
    ...
    if (asset == address(0)) {
      ...
    } else {
      _vaultAllowance[msg.sender][asset] -= amount; // Reduce allowance
      (bool success, bytes memory data) = asset.call(
        abi.encodeWithSignature("transfer(address,uint256)", to, amount)
      );
      require(success && (data.length == 0 || abi.decode(data, (bool))));
      ...
    }
    ...
  }

https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L209-L238

  function _transferOutV5(TransferOutData memory transferOutPayload) private {
    if (transferOutPayload.asset == address(0)) {
      ...
    } else {
      _vaultAllowance[msg.sender][
        transferOutPayload.asset
      ] -= transferOutPayload.amount; // Reduce allowance

      (bool success, bytes memory data) = transferOutPayload.asset.call(
        abi.encodeWithSignature(
          "transfer(address,uint256)",
          transferOutPayload.to,
          transferOutPayload.amount
        )
      );

      require(success && (data.length == 0 || abi.decode(data, (bool))));
    }
    ...
  }

https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code#F27#L185

    function transfer(address _recipient, uint256 _amount) external returns (bool) {
        _transfer(msg.sender, _recipient, _amount);
        return true;
    }

Proof of Concept

Using the StETH contract's following example, the following steps can occur for the described scenario.

https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code#F27#L19

 * StETH balances are dynamic and represent the holder's share in the total amount
 * of Ether controlled by the protocol. Account shares aren't normalized, so the
 * contract also stores the sum of all shares to calculate each account's token balance
 * which equals to:
 *
 *   shares[account] * _getTotalPooledEther() / _getTotalShares()
 *
 * For example, assume that we have:
 *
 *   _getTotalPooledEther() -> 10 ETH
 *   sharesOf(user1) -> 100
 *   sharesOf(user2) -> 400
 *
 * Therefore:
 *
 *   balanceOf(user1) -> 2 tokens which corresponds 2 ETH
 *   balanceOf(user2) -> 8 tokens which corresponds 8 ETH

1. At this moment, the total pooled Ether is 10 ETH and total share amount is 500 for the StETH contract.
2. user1 calls the depositWithExpiry function with his controlled address as the vault input to deposit 2 stETH, which is equivalent to 100 stETH shares.
3. user2 calls the depositWithExpiry function with a legitimate vault as the vault input to also deposit 2 stETH, which is equivalent to 100 stETH shares as well.
   • At this moment, the router holds 200 stETH shares in which 100 of them should belong to the legitimate vault.
4. After more on chain interactions with the StETH contract occur, the total pooled Ether changes to 19 ETH and total share amount changes to 1000 for the StETH contract.
5. user1 uses his controlled address to call the transferOut function to transfer 2 stETH to himself.
   • This does not revert because _vaultAllowance for his controlled address was 2 stETH before this transferOut function call.
   • user1 receives 2 * 1000 / 19 = 105 stETH shares and unfairly gains 5 more stETH shares because only 100 stETH shares should belong to him.
6. The router now holds 200 - 105 = 95 stETH shares.
   • The legitimate vault loses 5 stETH shares, which is a 5% loss, comparing to the 100 stETH shares that should belong to it.
   • user1 has stolen these 5 stETH shares from the legitimate vault.

Tools Used

Manual Review

Recommended Mitigation Steps

The depositWithExpiry function can be updated to revert if the vault input does not correspond to a legitimate vault. Moreover, for "vault-like" tokens like stETH, the router can be updated to store the respective _vaultAllowance by accumulating and decumulating share amounts instead of token amounts and transfer the respective token shares from the router when needed.

Assessed type

ERC20

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[c4-judge]

trust1995 marked the issue as satisfactory

[rbserver]

Hi @trust1995, thanks for judging!

This report and #85 describe the same issue, which I think is different from #55.

According to the C4 rule, if fixing the Root Cause (in a reasonable manner) would cause the finding to no longer be exploitable, then the findings are duplicates. Yet, the fix of #55 does not fix the issue described by this report and #85. Therefore, based on the C4 rule, would this report and #85 be reconsidered as a separate valid issue?

[c4-judge]

trust1995 marked the issue as not a duplicate

[c4-judge]

trust1995 marked the issue as duplicate of #85

[trust1995]

Agreed, separated out the other issues.

[c4-judge]

trust1995 changed the severity to 3 (High Risk)