Lines of code

https://github.com/code-423n4/2024-06-thorchain/blob/main/chain/ethereum/contracts/THORChain_Router.sol#L347-L375

Vulnerability details

Impact

transferOutAndCallV5 won't work properly with fee on transfer tokens leading to a swap error and approval accounting discrepancies.

Proof of Concept

The vault calls Router#transferOutAndCallV5 to Swap tokens and then send them to the recipient but is not handling correctly the fee of "fee on transfer tokens". The transferOutAndCallV5 first update the _vaultAllowance map, then transfer[!] the asset to the agregator and then execute the swapOutV5 with the amount before the transfer. So when executes the swapOutV5, the aggregator has the amount after the transfer but the router is asking to do the swap with the amount before the transfer leading to an error. Also there will be an approval accounting problem because the aggregator will approve the DEX with more tokens than needed.

Tools Used

VS code

Recommended Mitigation Steps

Is recommended to check the balance of Token before and after the transfer to see how much tokens were received and swap only the received, also the contract already has a function that handles fee on tranfer tokens: safeTransferFrom that returns the amount after the transfer,

so i would recommend to do in line _transferOutAndCallV5#370


    uint256 _startBalance = iERC20(aggregationPayload.fromAsset).balanceOf(aggregationPayload.target);

      // 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"
      );

      uint256 _finalAggregatorBalance = (iERC20(aggregationPayload.fromAsset).balanceOf(aggregationPayload.target) - _startBalance);

      // 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,
          _finalAggregatorBalance,
          aggregationPayload.toAsset,
          aggregationPayload.recipient,
          aggregationPayload.amountOutMin,
          aggregationPayload.payload,
          aggregationPayload.originAddress
        )
      );

Assessed type

Token-Transfer

code-423n4 / 2024-06-thorchain-findings

Router transferOutAndCallV5: Fee on transfer tokens lead to an error when swap and incorrect approval accounting #64