code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L190-L204 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L159-L174

Vulnerability details

Impact

True trustlessness is hard, but there's not much point in having open source smart contracts unless the goal is achieved completely. The moment a vector exists where a rug pull could occur a user should be rightly suspicious.

Although TimelockController is used on the timelock_address the same does not hold for the owner. The owner in the contract can withdraw ERC20 tokens or ether to themselves without going through the timelock, anytime they want (now or in the future).

Proof of Concept

See lines 159-174 and lines 190 - 204.
The onlyByOwnGov modifier makes sure that only the timelock can call the contract, or the deployer or owner of the contract.
The emergency functions,recoverEther and recoverERC20, are extremely powewr functions and should only be called by the timelock contract which is has governance mechnisms to prevent unilateral use. This does not hold for the owner who could always use these functions to completely drain the contract of funds.
The contract also lets the owner call setWithHoldRatio anytime they want, without a timelock. In combination with moveWithheldETH this presents another rug pull vector.

Tools Used

Manual Inspection

Recommended Mitigation Steps

Split the authority modifiers into two. Only the timelock_address should exclusively be able to recoverEth, recoverERC20, setWithHoldRatio and moveWithheldETH from the contract.

The two modifiers could be implemented as follows:

  modifier onlyTimeLock() {
        require(msg.sender == timelock_address , "Caller Not timelock");
        _;
    }

  modifier onlyOwner() {
        require(msg.sender == owner , "Caller Not owner");
        _;
    }

FortisFortuna commented 2 years ago

We are well aware of the permission structure. The owner will most likely be a large multisig. We mentioned the Frax Multisig in the scope too.

joestakey commented 2 years ago

Duplicate of #107

code-423n4 / 2022-09-frax-findings