Frax Ether Liquid Staking contest - OptimismSec's results

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");
        _;
    }

L-01: Round-off errors in frxETHMinter._submit can lead to loss of funds

Although the amount we are speaking about are tiny, perhaps consider rounding up when the withholdRatio is set to a non-zero value. Otherwise the possiblity exists that when extremely small msg.values are sent nothing is withheld.

The only check done for msg.value is to see if it is greater than zero on line 88. No other checks are done

require(msg.value != 0, "Cannot submit 0");

However, msg.value on line 96, is being divided by a constant RATIO_PRECISION which can lead to a result of zero for withheld_amt