LI.FI contest - sorrynotsorry's results

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/WithdrawFacet.sol#L20-L38 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/OwnershipFacet.sol#L8-L15

Vulnerability details

Medium Risk

Risk of centralization

Impact

Diamond owner has too many roles on setting the functions, initiating payable functions. If the Owner account is compromised, the assets may be drained in this trustless system.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/WithdrawFacet.sol#L20-L38 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/OwnershipFacet.sol#L8-L15

Tools Used

Static testing

Recommended Mitigation Steps

Team can consider building the owner account in DAO or multisig.

Lines of code

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L79-L85 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/WithdrawFacet.sol#L20-L38

Vulnerability details

Impact

Withdrawals and transferERC20 tokens are executed via transferERC20() and withdraw() functions. Since these functions calls with a fixed amount of gas, it's not yet guaranteed to reach to the destination if the receiver is a smart contract.

Proof of Concept

https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Libraries/LibAsset.sol#L79-L85 https://github.com/code-423n4/2022-03-lifinance/blob/699c2305fcfb6fe8862b75b26d1d8a2f46a551e6/src/Facets/WithdrawFacet.sol#L20-L38

Outlines are here : https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

Static testing

Recommended Mitigation Steps

Team can consider using call.value(amount)

QA Report (Low / Non-Critical)

• There is no zero address check in constructor function for the owner address in LiFiDiamond.sol
• Usage of deprecated safeApprove. Link
• Require statements are missing for addFunction() in LibDiamond.sol

Team can consider adding statements as;

require(_selector != 0, "Error msg here");
require(_facetAddress != address(0), "Error msg here");

• Remove sol-hints on various locations.
• Statement should be corrected to native asset rather than ether for correct guidance in LibAsset.sol#L42-L45
• Remove bug related todo at WithdrawFacet.sol#L29
• No amount check is carried out at LibAsset.sol inside transferNativeAsset()
• enforceHasContractCode in LibDiamond.sol should not be relied on if the target is a contract inside the construction. Reference is here
• Redundant if statement in LibDiamond.sol#196 since the statement is assumed as not succeeded.
• Emit event at LibDiamond.sol#79 can be considered to be thrown after initializeDiamondCut function success.