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

6 stars 4 forks source link

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

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#L36 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#L79 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.token is benign. This design flaw grants attackers the ability to deposit self-crafted worthless tokens in exchange for Diamond sending real valuable assets to the router.

Proof of Concept

Similar to other facets, AnyswapFacet does not check against the liability of tokens to swap. However, due to the extensive use of IAnyswapToken(_anyswapData.token).underlying(), the lack of check here becomes extremely vulnerable and can be easily attacked.

To demonstrate the exploit, we will focus on the startBridgeTokensViaAnyswap function and discuss the case of profitting by exchanging some garbage tokens to ether.

Below is the code covered in our attack. Notice while we focus our attack on startBridgeTokensViaAnyswap here, swapAndStartBridgeTokensViaAnyswap is also suscpetible 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 is 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 garbage tokens.

Our setup is approximately like this, we first choose an arbitrary benign _anyswapData.router, then craft a malicious _anyswapData.token which returns the address of a self-crafted token address(bogus) on the first call to IAnyswapToken(_anyswapData.token).underlying(), andIAnyswapRouter(_anyswapData.router).wNATIVE()` on the second.

Now we can trace what will happen throughout the process.

  1. underlyingToken will be set to address(bogus)
  2. since underlyingToken != wNATIVE (address(bogus) != wNATIVE), _anyswapData.amount of address(bogus) will be transfered from caller(attacker/us) to Diamond
  3. pass checks and proceed to enter _startBridge
  4. The second call to IAnyswapToken(_anyswapData.token).underlying() returns wNATIVE, so now underlyingToken == wNATIVE
  5. IAnyswapRouter(_anyswapData.router).anySwapOutNative{ value: _anyswapData.amount }(...) is called, _anyswapData.amount of ether is swapped in our name with the price of _anyswapData.amount bogus tokens

Same as our previous report on the unchecked router, it is possible to craft a _anyswapData.token that meets our need by either keeping an internal counter in the storage if call is used, or monitor remaining gas if staticcall is used. Either way, it won't be too hard to pull off the attack.

Once again, stealing ether isn't our only choice. The logic can be simply modified to steal tokens too

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

An ad-hoc fix of this is to call underlying only once and pass the results into _startBridge. However, while this makes it non trivial or probably impossible to attack the current contract, the root cause of the problem is yet to be solved. A full mitigation would likely be to adopt token whitelisting alongside with router` whitelist mentioned in previous report.

H3xept commented 2 years ago

Duplicate of #117

gzeoneth commented 2 years ago

Duplicate of #131 by the same warden.