code-423n4 / 2022-03-lifinance-findings

6 stars 4 forks source link

AnyswapFacet does not check whether passed `_anyswapData.router` is benign, allowing attackers to utilize this and steal any asset held by `Diamond` #131

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L35 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L37 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L74 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L80 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L131 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L136 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L137 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L149 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L151 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L159 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L161

Vulnerability details

Impact

AnyswapFacet does not check whether the passed _anyswapData.router is benign. This allows attackers to provide malicious routers to "exchange" tokens and native asset in an extremely favorable rate from Diamond, or potentially stealing all remaining tokens when chained with the LibAsset.approveERC20 bug discussed in a separate report.

Proof of Concept

Opposed to other contracts, AnyswapFacet does not have a fixed router contract explicitly assigned by Diamond owner. Instead, it allows users to pass the desired router along with other function arguments. This opens up a chance for attackers to craft a fake _anyswapData.router.

To demonstrate the exploit, we will focus on the startBridgeTokensViaAnyswap function and discuss the case of profiting by exchanging USDT to ether.

Below is the code covered in our attack. Notice while we focus our attack on startBridgeTokensViaAnyswap here, swapAndStartBridgeTokensViaAnyswap is also suscepetible to the same attack logic.

    function startBridgeTokensViaAnyswap(LiFiData memory _lifiData, AnyswapData calldata _anyswapData) public payable {
        address underlyingToken = IAnyswapToken(_anyswapData.token).underlying();
        if (_anyswapData.token != address(0) && underlyingToken != IAnyswapRouter(_anyswapData.router).wNATIVE()) {
            if (underlyingToken == address(0)) {
                underlyingToken = _anyswapData.token;
            }

            uint256 _fromTokenBalance = LibAsset.getOwnBalance(underlyingToken);
            LibAsset.transferFromERC20(underlyingToken, msg.sender, address(this), _anyswapData.amount);

            require(
                LibAsset.getOwnBalance(underlyingToken) - _fromTokenBalance == _anyswapData.amount,
                "ERR_INVALID_AMOUNT"
            );
        } else {
            require(msg.value == _anyswapData.amount, "ERR_INVALID_AMOUNT");
        }

        _startBridge(_anyswapData);

        ...
    }

    function _startBridge(AnyswapData memory _anyswapData) internal {
        // Check chain id
        require(block.chainid != _anyswapData.toChainId, "Cannot bridge to the same network.");
        address underlyingToken = IAnyswapToken(_anyswapData.token).underlying();

        if (underlyingToken == IAnyswapRouter(_anyswapData.router).wNATIVE()) {
            IAnyswapRouter(_anyswapData.router).anySwapOutNative{ value: _anyswapData.amount }(
                _anyswapData.token,
                _anyswapData.recipient,
                _anyswapData.toChainId
            );
            return;
        }

        if (_anyswapData.token != address(0)) {
            // Has underlying token?
            if (underlyingToken != address(0)) {
                // Give Anyswap approval to bridge tokens
                LibAsset.approveERC20(IERC20(underlyingToken), _anyswapData.router, _anyswapData.amount);

                IAnyswapRouter(_anyswapData.router).anySwapOutUnderlying(
                    _anyswapData.token,
                    _anyswapData.recipient,
                    _anyswapData.amount,
                    _anyswapData.toChainId
                );
            } else {
                // Give Anyswap approval to bridge tokens
                LibAsset.approveERC20(IERC20(_anyswapData.token), _anyswapData.router, _anyswapData.amount);

                IAnyswapRouter(_anyswapData.router).anySwapOut(
                    _anyswapData.token,
                    _anyswapData.recipient,
                    _anyswapData.amount,
                    _anyswapData.toChainId
                );
            }
        }
    }

Imagine a scenario where there are currently some ether in the Diamond contract (or we can trigger come bridges to send ether to the contract either due to timeout/expiry or by simply fulfilling a swap). Our target is to siphon those ether by depositing USDT on a 1:1 basis.

Our argument setup is approximately like this, we choose a _anyswapData.token that has USDT as IAnyswapToken(_anyswapData.token).underlying(), then design a malicious _anyswapData.router that has IAnyswapRouter(_anyswapData.router).wNATIVE() return address(0) on first call and address(USDT) on second call. We also set _anyswapData.amount to be equal to current address(Diamond).balance.

Now we can trace what will happen throughout the process.

  1. underlyingToken will be set to USDT
  2. since underlyingToken != wNATIVE (address(USDT) != address(0)), _anyswapData.amount of USDT will be transferred from caller(attacker/us) to Diamond
  3. pass checks and proceed to enter _startBridge
  4. The second call of wNATIVE returns address(USDT), so now underlyingToken == wNATIVE
  5. IAnyswapRouter(_anyswapData.router).anySwapOutNative{ value: _anyswapData.amount }(...) is called, we now can walk away with _anyswapData.amount of ether

The exploit is pretty simple, but we still need to deal with how to have the fake router return different wNATIVE for the first and second call. If we assume that the call is done with call opcode, it is pretty easy to just keep an internal storage in the fake router contract to decide on what value to return. However, if staticcall is used, we can no longer change storage value within the fake router. Thankfully under this case, it is still possible to utilize remaining gas to decide when to switch from returning address(0) to address(USDT).

The attack can also be adapted to stealing tokens by depositing ether, under cases where valuable tokens such as WBTC are held by the Diamond contract. In fact, when used together with the LibAsset.approveERC20 function which approves UINT256_MAX (instead of amount specified in argument) to the spender, it is possible specify a 0 _anyswapData.amount and proceed to transfer all tokens from Diamond to attacker.

Overall, this lack of check on router fully compromises any asset stored in the Diamond contract.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

While one of the strengths of anySwap is its flexibility, we have shown in this context that flexibility equates vulnerability. The easiest way to fix this is to adopt a whitelist where Diamond contract owners may add/remove trusted routers.

H3xept commented 2 years ago

Duplicate of #12

gzeoneth commented 2 years ago

Duplicate of #117