Some of token's underlying assets for corresponding vault can fail to be transferred when such token is a rebasing token or token with airdrops, such as AMPL

[howlbot-integration]

Lines of code

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#L438-L453
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
https://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L304-L389

Vulnerability details

Impact

Based on this protocol's token whitelist, this protocol needs to support rebasing tokens and tokens with airdrops, such as AMPL. Balances of such tokens' holders can be changed when respective rebasing or airdrop event is triggered. For example, as shown by AMPL's balanceOf function below, the AMPL holders' token balances depend on _gonsPerFragment, which can be changed when AMPL's rebase function below is called.

https://etherscan.io/address/0xd0e3f82ab04b983c05263cf3bf52481fbaa435b1#code#F1#L169

    function balanceOf(address who) external view override returns (uint256) {
        return _gonBalances[who].div(_gonsPerFragment);
    }

https://etherscan.io/address/0xd0e3f82ab04b983c05263cf3bf52481fbaa435b1#code#F1#L107

    function rebase(uint256 epoch, int256 supplyDelta)
        external
        onlyMonetaryPolicy
        returns (uint256)
    {
        if (supplyDelta == 0) {
            emit LogRebase(epoch, _totalSupply);
            return _totalSupply;
        }

        if (supplyDelta < 0) {
            _totalSupply = _totalSupply.sub(uint256(supplyDelta.abs()));
        } else {
            _totalSupply = _totalSupply.add(uint256(supplyDelta));
        }

        if (_totalSupply > MAX_SUPPLY) {
            _totalSupply = MAX_SUPPLY;
        }

        _gonsPerFragment = TOTAL_GONS.div(_totalSupply);
        ...
    }

For such token, it is possible that the _deposit function below is called before the token's rebasing or airdrop event is triggered. This would cause _vaultAllowance to be inaccurate for such token when the token's rebasing or airdrop event is later triggered. Due to the inaccurate _vaultAllowance for the corresponding token and vault, such token's underlying assets for such vault can be lost when all of such _vaultAllowance is transferred from the router.

For instance, when the following _deposit function for AMPL is called for the first time and AMPL's rebase function has not been called, based on the transferFrom function below and balanceOf function of AMPL, _vaultAllowance for AMPL and the corresponding vault would be increased to the router's _gonBalances for such vault dividing by _gonsPerFragment at that moment.

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
    }
    ...
  }

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

  function safeTransferFrom(
    address _asset,
    uint _amount
  ) internal returns (uint amount) {
    uint _startBal = iERC20(_asset).balanceOf(address(this));
    (bool success, bytes memory data) = _asset.call(
      abi.encodeWithSignature(
        "transferFrom(address,address,uint256)",
        msg.sender,
        address(this),
        _amount
      )
    );
    require(success && (data.length == 0 || abi.decode(data, (bool))));
    return (iERC20(_asset).balanceOf(address(this)) - _startBal);
  }

https://etherscan.io/address/0xd0e3f82ab04b983c05263cf3bf52481fbaa435b1#code#F1#L270

    function transferFrom(
        address from,
        address to,
        uint256 value
    ) external override validRecipient(to) returns (bool) {
        _allowedFragments[from][msg.sender] = _allowedFragments[from][msg.sender].sub(value);

        uint256 gonValue = value.mul(_gonsPerFragment);
        _gonBalances[from] = _gonBalances[from].sub(gonValue);
        _gonBalances[to] = _gonBalances[to].add(gonValue);
        ...
    }

Later, AMPL's rebase function is called, which increases its _totalSupply and decreases its _gonsPerFragment. Afterwards, when the corresponding vault calls the following transferOut, _transferOutV5, or _transferOutAndCallV5 function, the maximum AMPL balances that can be transferred from the router would be the _vaultAllowance for AMPL and such vault. Since such _vaultAllowance equals the router's _gonBalances for the corresponding vault dividing by the previous _gonsPerFragment, multiplying such _vaultAllowance by the new lower _gonsPerFragment would be less than the router's _gonBalances for such vault; thus, when AMPL's transfer function below is called, some of the router's _gonBalances for the corresponding vault cannot be transferred but all of the _vaultAllowance for AMPL and such vault is used and removed. As a result, the portion of the router's _gonBalances for the corresponding vault, which fails to be transferred, is lost.

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 {
    uint safeAmount;
    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))));
      safeAmount = amount;
    }
    ...
  }

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://github.com/code-423n4/2024-06-thorchain/blob/5b91b5c6683a222b0ce046533515e301c9699d74/chain/ethereum/contracts/THORChain_Router.sol#L304-L389

  function _transferOutAndCallV5(
    TransferOutAndCallData calldata aggregationPayload
  ) private {
    if (aggregationPayload.fromAsset == address(0)) {
      ...
    } else {
      _vaultAllowance[msg.sender][
        aggregationPayload.fromAsset
      ] -= aggregationPayload.fromAmount; // Reduce allowance

      // send ERC20 to aggregator contract so it can do its thing
      (bool transferSuccess, bytes memory data) = aggregationPayload
        .fromAsset
        .call(
          abi.encodeWithSignature(
            "transfer(address,uint256)",
            aggregationPayload.target,
            aggregationPayload.fromAmount
          )
        );

      require(
        transferSuccess && (data.length == 0 || abi.decode(data, (bool))),
        "Failed to transfer token before dex agg call"
      );

      // add test case if aggregator fails, it should not revert the whole transaction (transferOutAndCallV5 call succeeds)
      // call swapOutV5 with erc20. if the aggregator fails, the transaction should not revert
      (bool _dexAggSuccess, ) = aggregationPayload.target.call{value: 0}(
        abi.encodeWithSignature(
          "swapOutV5(address,uint256,address,address,uint256,bytes,string)",
          aggregationPayload.fromAsset,
          aggregationPayload.fromAmount,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );
      ...
    }
  }

https://etherscan.io/address/0xd0e3f82ab04b983c05263cf3bf52481fbaa435b1#code#F1#L223

    function transfer(address to, uint256 value)
        external
        override
        validRecipient(to)
        returns (bool)
    {
        uint256 gonValue = value.mul(_gonsPerFragment);

        _gonBalances[msg.sender] = _gonBalances[msg.sender].sub(gonValue);
        _gonBalances[to] = _gonBalances[to].add(gonValue);
        ...
    }

Proof of Concept

The following steps can occur for the described scenario.

1. At this moment, for AMPL:
   • TOTAL_GONS is 1.157920892373162e77;
   • _totalSupply is 268765777507592648;
   • _gonsPerFragment is 1.157920892373162e77 / 268765777507592648 = 4.308289928543639e59.
2. A user calls the _deposit function to deposit 1e13 wei AMPL for Vault A.
   • _vaultAllowance for AMPL and Vault A equals 1e13.
   • The router's _gonBalances for Vault A equals 1e13 * 4.308289928543639e59 = 4.308289928543639e72.
3. AMPL's rebase function is called to increase _totalSupply by 1e15.
   • _totalSupply equals 268765777507592648 + 1e15 = 269765777507592640.
   • The new _gonsPerFragment equals 1.157920892373162e77 / 269765777507592640 = 4.292319444932454e59.
4. Vault A calls the _transferOutV5 function to transfer all of the _vaultAllowance for AMPL and Vault A that is 1e13.
   • _vaultAllowance for AMPL and Vault A is reduced to 0.
   • The transferred portion of the router's _gonBalances for Vault A equals 1e13 * 4.292319444932454e+59 = 4.292319444932454e72.
   • The portion of the router's _gonBalances for Vault A, which fails to be transferred, equals 4.308289928543639e72 - 4.292319444932454e72 = 1.5970483611185176e70.

Tools Used

Manual Review

Recommended Mitigation Steps

Like many other protocols, this protocol can consider to not support rebasing tokens and tokens with airdrops, such as AMPL. If this protocol wants to support these tokens, it needs to communicate with its users about the shortcomings of these tokens in this protocol, such as these tokens' airdrops can be lost. Alternatively, _vaultAllowance for these tokens can be updated to keep track of these tokens' underlying assets held by the router for the corresponding vaults in order to transfer the underlying assets instead of the balances of these tokens.

Assessed type

ERC20

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as not a duplicate

[c4-judge]

trust1995 marked the issue as duplicate of #85

[c4-judge]

trust1995 changed the severity to 3 (High Risk)