search

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

6 stars 4 forks source link

QA Report #139

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Summary

We list 5 low-critical findings and 1 non-critical finding here:

In summary of recommended security practices, it's better to check native assets, chainId and 3rd party bridge versions to avoid abuse. Also, it’s better to follow EIP-2535 proposal and check the exact amount for better practices.

(LOW) AnyswapFacet implicitly rejects address(0) as _anyswapData.token and reverts without warning

Impact

AnyswapFacet has a different interface for accepting native assets compared to other facets. This might lead to confusion and misuse of the facet, and result in loss of gas for contract users.

The logic of deciding which token to use in Anyswap is more complex than other facets. Overall, the decision is done twice, first for checking (and transferring funds) from the user, and then for deciding which Anyswap router function to call.

logic for startBridgeTokensViaAnyswap

  1. if _anyswapData.token == address(0), native asset is used
  2. if _anyswapData.token.underlying() == _anyswapData.router.wNATIVE(), native asset is used
  3. if _anyswapData.token.underlying() == address(0), _anyswapData.token is used
  4. if _anyswapData.token.underlying() is used

logic for _startBridge

  1. if _anyswap.token.underlying() == _anyswapData.router.wNATIVE(), call anySwapOutNative()
  2. if _anyswapData.token == address(0), do nothing (note that this path would revert upon calling IAnyswapToken(_anyswapData.token).underlying() in _startBridge
  3. if _anyswapData.token.underlying() == address(0), call anySwapOut()
  4. call anySwapOutUnderlying()

The main mismatch rises when _anyswapData.token == address(0), startBridgeTokensViaAnyswap accepts those cases and uses native asset, while _startBridge does not handle this and implicitly reverts.

Since other facets usually use address(0) as native asset, we judged that users might be confused by the behaviour of AnyswapFacet, especially since it does not reject address(0) early in call, and only reverts at a relatively late stage.

Proof of Concept

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#L49 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#L95 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#L145

Recommended Mitigation Steps

It would be more appropriate to follow the same pattern as other facets, and add code to handle address(0) in _startBridge. Or at the very least, disallow address(0) early in startBridgeTokensViaAnyswap and swapAndStartBridgeTokensViaAnyswap and make it explicit that address(0) as token is not allowed.

(LOW) CBridgeFacet allows more than _cBridgeData.amount of native assets to be sent to it

Impact

Allowing excessive native assets to be sent is unnecessary, and would potentially lead to loss of the caller's asset.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/CBridgeFacet.sol#L68

Recommended Mitigation Steps

Use == rather than >=

require(msg.value == _cBridgeData.amount, "ERR_INVALID_AMOUNT");

(LOW) Anyswap router version confusion

Impact

Anyswap router used in deployment/test is not compatible with AnyswapFacet function calls.

The provided Anyswap router address in test/facet/AnyswapFacet.test.ts and config/anyswap.ts are both implementing AnyswapV4Router. However, anySwapOutNative is only implemented in later versions of Anyswap router (e.g. AnyswapV6Router). While this is not strictly included in audit scope, we deem it appropriate to bring attention to the mismatch, in case of incorrect deployment in the future.

Proof of Concept

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/config/anyswap.ts#L14 https://github.com/code-423n4/2022-03-lifinance/blob/main/test/facets/AnyswapFacet.test.ts#L14

Recommended Mitigation Steps

Use a newer Anyswap router address in deployment and test script instead.

(LOW) NXTPFacet does not check whether _nxtpData.invariantData.sendingChainId equals current chainId

Impact

Diamond might be abused to serve as router and call prepare on the receiving chain.

In the nxtp protocol, prepare is called by the sender in the sending chain and by router in the receiving chain. Diamond only intended to serve as the sender, but since it does not check _nxtpData.invariantData.sendingChainId against the current chainId, it is possible to abuse the field and call prepare as a router instead.

While the above abuse is theoretically possible, there are several checks in the nxtp protocol that must be satisfied in order to perform such actions. Including getting Diamond approved as a valid router, making bids in place of Diamond and so on. Overall, the trick is quite difficult to pull off.

Proof of Concept

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 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L185

Recommended Mitigation Steps

Validate _nxtpData.invariantData.sendingChainId against chainId.

(LOW) NXTPFacet does not check _nxtpData.invariantData.callTo and _nxtpData.invariantData.callDataHash equals current chainid

Impact

Intended fulfill call back can be skipped or replaced with other functions.

The design of the NXTPFacet is that the two fulfill functions will be called on the receiving chain. However, there are no checks against whether the two helper functions completeBridgeTokensViaNXTP and swapAndCompleteBridgeTokensViaNXTP are passed into as calldata, and whether Diamond is even assigned as callTo. This allows users to assign arbitrary callbacks on the receiving chain.

While this does not pose any direct threats, it would be appropriate to check the two calling fields and conform to contract usage design.

Proof of Concept

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 https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/NXTPFacet.sol#L185

Recommended Mitigation Steps

Validate _nxtpData.invariantData.callTo and _nxtpData.invariantData.callDataHash against the intended callback functions.

(Non-Critical) initializeDiamondCut implementation disagrees with EIP-2535

Impact

The original proposal of Diamond specifies that _calldata is allowed to carry additional information even when _init is address(0). Current implementation disagrees with the proposal.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L185

Recommended Mitigation Steps

Refer to EIP-2535: https://eips.ethereum.org/EIPS/eip-2535

If the _init value is address(0) then _calldata execution is skipped. In this case _calldata can contain 0 bytes or custom information.
H3xept commented 2 years ago

Re anyswap check for 0x0

Fixed in lifinance/lifi-contracts@f35ed79a266a69b363d72332b7861d15d18b98cb

H3xept commented 2 years ago

CBridgeFacet amount -- Fixed in lifinance/lifi-contracts@a8d6336c2ded97bdbca65b64157596b33f18f70d

H3xept commented 2 years ago

Re Anyswap router mismatch

I'm reasonably certain AnyswapRouterV4 does implement anySwapNative as it inherits from V3 and the function is present there (0x332730a4F6E03D9C55829435f10360E13cfA41Ff)