code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Description

Changing critical addresses in contracts should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e., grants ownership) and the second transaction (from the new address) replaces the old address with the new one (i.e., claims ownership). This allows recovering from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Impact

If the contract does not validate address changes properly, any mistake from the owner when changing the address could be fatal and the contract might be lost forever.

Proof of Concept

We can notice in the function transferOwnership at (https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Facets/OwnershipFacet.sol#L8-L11) calls the function setContractOwner at https://github.com/code-423n4/2022-03-lifinance/blob/main/src/Libraries/LibDiamond.sol#L44-L49 which directly update the new admin address. The previousOwner is only used for emitting.

Recommended Mitigation Steps

Enable a two-step process for critical address changes. In the first step, the new owner's address should be submitted, and in the next transaction that addresses can be claimed.

H3xept commented 2 years ago

Implemented in lifinance/lifi-contracts@221fe883f705635feebb2af64a028f30d05afbf8

gzeoneth commented 2 years ago

Downgrading to Low/QA.