code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/b58c9b72f5fe8fab81f7436504e7daf60fd124e3/src/contracts/FraxlendPairCore.sol#L46 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendPairDeployer.sol#L44 https://github.com/code-423n4/2022-08-frax/blob/92a8d7d331cc718cd64de6b02515b554672fb0f3/src/contracts/FraxlendWhitelist.sol#L30

Vulnerability details

Single-step process for critical ownership transfer/renounce is risky

Impact

The following contracts and functions, allow owners to interact with core functions such as:

FraxlendPair.sol functions that manage permissions, fees and withdraws: setTimeLock, changeFee (affected because setTimeLock), withdrawFees, setSwapper
FraxlendPairDeployer.sol function that manage the creationCode for the Fraxlend Pair: setCreationCode
FraxlendWhitelist.sol functions: setSwapper setRateContractWhitelist setFraxlendDeployerWhitelist

Given that FraxlendWhitelist, FraxlendPairDeployer and FraxlendPairCore are derived from Ownable, the ownership management of this contract defaults to Ownable ’s transferOwnership() and renounceOwnership() methods which are not overridden here.

Such critical address transfer/renouncing in one-step is very risky because it is irrecoverable from any mistakes

Scenario: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyOwner() functions forever, which includes the changing of various critical addresses and parameters. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately.

When noticed, due to a failing onlyOwner() or onlyOwnerOrAssetManager() function call, it will force the redeployment of these contracts and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in the protocol and incur a significant reputational damage.

Github permalinks

Proof of concept

See similar High Risk severity finding from Trail-of-Bits Audit of Hermez. https://github.com/trailofbits/publications/blob/master/reviews/hermez.pdf See similar Medium Risk severity finding from Trail-of-Bits Audit of Uniswap V3: https://github.com/Uniswap/v3-core/blob/main/audits/tob/audit.pdf

Recommended steps

Recommend overriding the inherited methods to null functions and use separate functions for a two-step address change:

Approve a new address as a pendingOwner
A transaction from the pendingOwner address claims the pending ownership change.

This mitigates risk because if an incorrect address is used in step (1) then it can be fixed by re-approving the correct address. Only after a correct address is used in step (1) can step (2) happen and complete the address/ownership change.

Also, consider adding a time-delay for such sensitive actions. And at a minimum, use a multisig owner address and not an EOA.

DrakeEvans commented 2 years ago

Contract does not expend gas to protect against user error.

gititGoro commented 2 years ago

This type of report is helpful but falls under the category of "user error could lead to problems" which is not something that can be protected against in deployment and config situations. Also this protection can be deferred to higher realms such as in a DAO which owns these contracts. In fact, it may be argued that the DAO is the more appropriate level at which transfer rules should be specified and the contracts should only facilitate the wishes of the DAO. Downgrading to QA.