code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L62 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L100 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L136 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L140 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L142 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L155 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L167

Vulnerability details

Impact

Users can accidentally select a non-registered _hopData.asset, and the transaction will only implicit revert upon the final call to IHopBridge(bridge).sendToL2 or IHopBridge(bridge).swapAndSend. Current revert relies on implicitly enforced contract length check by solc, and is not reliable in general.

Proof of Concept

HopFacet fetches target bridge by looking up its storage using _hopData.asset as key. While this prevents use of malicious tokens as in other facets, it is essential to check whether an asset is actually registered before use. The current implementation lacks this check, and as a result, HopFacet will fetch native asset as sendingAssetId, and address(0) as bridge.

        address sendingAssetId = _bridge(_hopData.asset).token;

        address bridge;
        if (s.hopChainId == 1) {
            bridge = _bridge(_hopData.asset).bridge;
        } else {
            bridge = _bridge(_hopData.asset).ammWrapper;
        }

The contract then proceeds to call IHopBridge(bridge).sendToL2{ value: value }(...) or IHopBridge(bridge).swapAndSend{ value: value }(...), which thankfully reverts due to solidity's implicit check on code length for interface functions.

If such a check were to be removed in later solc versions, users native assets would be sent to address(0), effectively burnt forever.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Validate _hopData.asset by checking at least one of _bridge(_hopData.asset).bridge and _bridge(_hopData.asset).ammWrapper is not address(0) early in function call.

H3xept commented 2 years ago

Fixed in lifinance/lifi-contracts@7da994262d90dc173823588d43ce74a0e5f5576a

maxklenk commented 2 years ago

We "disagree with severity" as we define the bridges via our admin api and can therefore ensure that not address(0) is set. Also as mentioned in the issue the function would revert if the user passed an invalid asset which is not bridgeable by hop.

gzeoneth commented 2 years ago

Downgrading to Low/QA.