QA Report

[code423n4]

1. HopFacet's initHop doesn't check the lengths of the argument arrays

Impact

On calling with arrays of different lengths various malfunctions are possible as the arrays are used as given. System then will fail with low level array access message.

Proof of Concept

HopFacet's initHop:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/HopFacet.sol#L40-L50

Recommended Mitigation Steps

Require that _tokens and _bridgeConfigs arrays' lengths match

2. Floating pragma is used in most contracts

As different compiler versions have critical behavior specifics if the contracts get accidentally deployed using another compiler version compared to the one they were tested with, various types of undesired behavior can be introduced

Proof of Concept

pragma solidity ^0.8.7 is used in most contracts, for example:

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

Recommended Mitigation Steps

Consider fixing the version to 0.8.x across all the codebase, for example set x to 10

3. Assert is used instead of require in WithdrawFacet

Impact

Assert will consume all the available gas, providing no additional benefits when being used instead of require, which both returns gas and allows for error message

Proof of Concept

There are two uses of assert:

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

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

Recommended Mitigation Steps

Using assert in production isn't recommended, consider substituting it with require in all the cases

4. User facing Facets miss emergency lever

Impact

If there be any emergency with system contracts or outside infrastructure, there is no way to temporary stop the operations.

Proof of Concept

All core user-facing contracts do not have pausing functionality for new operation initiation.

For example, both AnyswapFacet's user facing functions, bridge and swap-bridge, cannot be temporary paused:

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#L74

Recommended Mitigation Steps

Consider making all end user facing Facets pausable.

For example, by using OpenZeppelin's approach:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/Pausable.sol

5. No zero-address checks for new owner address

Impact

If Diamond owner be set to zero by mistake the range of system managing functions become unreachable. Particularly, mistakenly sent funds retrieval and DEX white list management become unavailable.

Depending on the stage when it happens it might be needed to redeploy the system.

Proof of Concept

OwnershipFacet.transferOwnership doesn't check the _newOwner:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/OwnershipFacet.sol#L8-L11

LibDiamond.setContractOwner doesn't check the _newOwner:

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

Recommended Mitigation Steps

Consider requiring _newOwner to be non-zero at one of the levels.

If ownership renounce is in scope it's recommended to add separate methods for it as it's crucial operation for the system that should be more clearly tracked. Also, separate function reduces the probability of setting owner to zero by mistake.

6. Diamond ownership is transferred in one go

Impact

One step process offers no protection for the cases when ownership transfer is performed mistakenly or with any malicious intent.

Adding a modest complexity of an additional step and a delay is a low price to pay for having time to evaluate the change.

Proof of Concept

OwnershipFacet set a new owner immediately:

https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/OwnershipFacet.sol#L8-L11

Recommended Mitigation Steps

Consider utilizing two-step ownership transferring process (proposition and acceptance in separate actions) with a noticeable delay between the steps to enforce the transparency and stability of the system

7. DiamondCutFacet can immediately replace any functionality of the system

Impact

Any malfunction can be introduced by the changes either by mistake or with a malicious intent. Say an update can introduce a way to rug the funds.

As this possibility is visible from the contract code it posses the reputational risk even being a part of base Diamond design.

The rationale here is that the project could have, but didn't introduced the measures to mitigate it at least partially.

For example, any integrations and programmatic usage by other projects will be questioned as the very same call can produce quite different results and it's not trivial to setup the checks to control for its validity.

Proof of Concept

DiamondCutFacet's diamondCut let owner to add/remove/replace any functions:

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

Which is done via LibDiamond.diamondCut:

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

Now these calls aren't anyhow constrained, so any change be effective immediately.

Recommended Mitigation Steps

Consider adding transparent constraints on the usage of system upgrade functionality, for example an introduction of two step process with a delay.

If the upgradability is meant to be for alpha-beta stages only consider the introduction of such stages to the code, with disabling the upgrades when the stage progresses past beta, for example.

Further system evolution can be achieved with deployment of additional Diamonds.

[H3xept]

Re No zero-address checks for new owner address

Duplicate of #192

[H3xept]

Re HopFacet's initHop doesn't check the lengths of the argument arrays

Duplicate of #71

[H3xept]

Re Assert is used instead of require

Duplicate of #165

[H3xept]

Re Diamond ownership is transferred in one go

Duplicate of #143