search

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

6 stars 4 forks source link

Lack of chainID validation in all contracts #142

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#L25 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L21 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L21 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L29 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L46 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L85

Vulnerability details

Description

All the blockchain networks have an identifier called Chain ID. This helps the code identify where to send the transactions. It's just an additional way to tell chains apart. It is required for the chain to operate in general - e.g., it's required when signing transactions, meaning transactions signed on the ETH network end up with a different hash than those signed on ETC. This is used to prevent replay attacks. Here the smart contract allows swapping from any chain ID. The chain respective to the chainID can be a dead chain or a rouge chain, and hence there should be some kind of whitelisting.

Impact

Due to missing input validation on the Chain ID, the transactions may revert, and the funds may be lost if the user enters the wrong chain ID.

Proof of Concept

Below is the list of functions which is accepting chain ID as input but they are not validating if it is a valid chain ID

  1. toChainId(https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/AnyswapFacet.sol#L25) is used in startBridgeTokensViaAnyswap & swapAndStartBridgeTokensViaAnyswap
  2. cBridgeChainId and dstChainId (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L21) is used in functions initCbridge & startBridgeTokensViaCBridge & swapAndStartBridgeTokensViaCBridge
  3. destinationChainId is used in function swapTokensGeneric at (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/GenericSwapFacet.sol#L22)
  4. hopChainId (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L21) and chainId (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L29) used in function initHop startBridgeTokensViaHop & swapAndStartBridgeTokensViaHop
  5. destinationChainId in functions startBridgeTokensViaNXTP (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L46) & swapAndStartBridgeTokensViaNXTP (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L85)

Recommended Mitigation Steps

The possible fix could be to have a whitelist array of all the valid chain IDs and have input validation on all the user inputs accepting those chain IDs. Only allow whitelisted chain IDs to be accepted. Another possible solution is to read the value of chainID opcode using inline assembly, which was defined in EIP-1344, and check if it is a valid chainId. But this seems like a very hard way and will require a lot of hacks.

maxklenk commented 2 years ago

Thank you very much for pointing out that we do not validate the chainIds passed to the bridges we support. This is true for most of the data we pass along to the bridges. The bridge themself are supposed to validate their inputs and revert if invalid data is passed. This is especially true for chainIds as the bridges constantly add support for more chains and each bridge would require its own list of allowed chainIds. We for sure maintain this information in our backend and generate only valid contract calls.

gzeoneth commented 2 years ago

Invalid submission.