Frax Ether Liquid Staking contest - Aymen0909's results

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L166-L174 https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L191-L196

Vulnerability details

Impact

In the frxETHMinter contract both the owner and governance timelock have the power to call the functions moveWithheldETH and recoverEther, those functions allow the transfer of the ETH from frxETHMinter to the owner or a given account, this means that the owner can easily call one of those functions to withdraw all the ETH balance and run with it which is basically a Rug Pull.

The impact of this is the following :

• For the moveWithheldETH function the impact is lower as even if the owner rug pull the funds only the currentWithheldETH will be transferred which will of course remove the market liquidity but the other ETH funds intended for staking will be safe.

• In the case of the recoverEther function the impact is much higher as the owner can call this function at any time and withdraw the full frxETHMinter contract ETH balance this will stop completely the staking process as there will be no ETH to be deposited and the users who held sfrxETHToken will not receive any rewards and won't be able to get their ETH back.

Proof of Concept

Both moveWithheldETH and recoverEther functions have the onlyByOwnGov modifier which means that they can be called either by the governance timelock or the owner at any time :

function moveWithheldETH

    function moveWithheldETH(address payable to, uint256 amount) external onlyByOwnGov {
        require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");
        currentWithheldETH -= amount;

        (bool success,) = payable(to).call{ value: amount }("");
        require(success, "Invalid transfer");

        emit WithheldETHMoved(to, amount);
    }

function recoverEther

    function recoverEther(uint256 amount) external onlyByOwnGov {
        (bool success,) = address(owner).call{ value: amount }("");
        require(success, "Invalid transfer");

        emit EmergencyEtherRecovered(amount);
    }

Tools Used

Visual audit

Recommended Mitigation Steps

There are two solution to avoid the risk of a Rug pull :

• The receiver of ETH funds in both moveWithheldETH and recoverEther functions should be set to the protocol treasury so even if the functions are called by the owner the ETH will be transferred to the treasury and not to another address.

• The frxETHMinter contract should only allow the governance timelock to call the functions moveWithheldETH and recoverEther, this can be done by replacing the onlyByOwnGov modifier with an onlyByGov modifier.

QA Report

Summary

Findings

1- Immutable state variables lack zero address checks :

Constructor should check the values written in an immutable state variables in this case addresses to verify that it is not the zero address (address(0))

Impact - Low Risk

Proof of Concept

Instances include:

File: src/frxETHMinter.sol

depositContract = IDepositContract(depositContractAddress);

frxETHToken = frxETH(frxETHAddress);

sfrxETHToken = IsfrxETH(sfrxETHAddress);

Mitigation

Add non-zero address checks in the constructor for the instances aforementioned.

2- Redundant check in the submitAndDeposit function :

The check for the amount received after calling sfrxETHToken.deposit(msg.value, recipient); is redundant as the deposit function from the solmat ERC4626 contract already contain a check for the non-zero shares amount, so it will automaticaly revert if the returned amount (shares) is 0.

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/frxETHMinter.sol

require(sfrxeth_recieved > 0, 'No sfrxETH was returned');

Mitigation

Remove the check aforementioned because it's redundant.

3- Non-library/interface files should use fixed compiler versions, not floating ones :

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

check this source

Impact - NON CRITICAL

Proof of Concept

All contracts use a floating solidity version :

pragma solidity ^0.8.0;

Mitigation

Lock the pragma version to the same version as used in the other contracts and also consider known bugs (https://github.com/ethereum/solidity/releases) for the compiler version that is chosen.

Pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or EthPM package. Otherwise, the developer would need to manually update the pragma in order to compile it locally.

4- Named return variables not used anywhere in the function :

When Named return variable are declared they should be used inside the function instead of the return statement or if not they should be removed to avoid confusion.

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/frxETHMinter.sol

returns (uint256 shares)

File: src/sfrxETH.sol

returns (uint256 shares)

returns (uint256 assets)

Mitigation

Either use the named return variables inplace of the return statement or remove them.

5- public functions not called by the contract should be declared external instead :

Impact - NON CRITICAL

Proof of Concept

Instances include:

File: src/ERC20/ERC20PermitPermissionedMint.sol

function minter_burn_from(address b_address, uint256 b_amount) public

function minter_mint(address m_address, uint256 m_amount) public

function addMinter(address minter_address) public

function removeMinter(address minter_address) public

function setTimelock(address _timelock_address) public

Mitigation

Declare the functions aforementioned external.

Gas Optimizations

Summary

Findings

1- Optimize the removeValidator function to save gas :

In the removeValidator function when dont_care_about_ordering is set to false, the function will delete the whole validators array and then reset it all over again.

This could be optimized by shifting the element to be removed to the last and then using pop on it, in this way the validators array will be reset only if the deleted element is the first one but in all other cases the for loop will not run through the whole validators array, so the number of iterations will be less than in the reset method thus saving gas.

The instance occurs in the code below :

File: src/frxETHMinter.sol Line 106-118

else {
    // Save the original validators
    Validator[] memory original_validators = validators;

    // Clear the original validators list
    delete validators;

    // Fill the new validators array with all except the value to remove
    for (uint256 i = 0; i < original_validators.length; ++i) {
        if (i != remove_idx) {
            validators.push(original_validators[i]);
        }
    }

This should be modified as follow :

else {
    uint256 len = validators.length - 1;
    for (uint256 i = remove_idx; i < len; i++){
            validators[i] = validators[i+1];
        }
    validators.pop();
    }

2- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There is 1 instance of this issue:

File: src/frxETHMinter.sol Line 168

currentWithheldETH -= amount;

The above operation cannot underflow due to the check require(amount <= currentWithheldETH, "Not enough withheld ETH in contract"); and should be modified as follows :

unchecked {
    currentWithheldETH -= amount;
}

3- Using bool for storage incurs overhead :

Use uint256(1) and uint256(2) instead of true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

There are 2 instances of this issue:

File: src/frxETHMinter.sol Line 49

bool public submitPaused;

File: src/frxETHMinter.sol Line 50

bool public depositEtherPaused;

4- <array>.length should not be looked up in every loop in a for loop :

There are 2 instances of this issue:

File: src/ERC20/ERC20PermitPermissionedMint.sol Line 84

for (uint i = 0; i < minters_array.length; i++)

File: src/OperatorRegistry.sol Line 114

for (uint256 i = 0; i < original_validators.length; ++i)

5- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

There are 2 instances of this issue:

File: src/frxETHMinter.sol Line 97

currentWithheldETH += withheld_amt;

File: src/frxETHMinter.sol Line 168

currentWithheldETH -= amount;

6- Use custom errors rather than require()/revert() strings to save deployments gas :

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information.

There are 20 instances of this issue :

File: src/ERC20/ERC20PermitPermissionedMint.sol

41       require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");
46       require(minters[msg.sender] == true, "Only minters");
66       require(minter_address != address(0), "Zero address detected");
68       require(minters[minter_address] == false, "Address already exists")
77       require(minter_address != address(0), "Zero address detected");
78       require(minters[minter_address] == true, "Address nonexistant"); 
95       require(_timelock_address != address(0), "Zero address detected"); 

File: src/frxETHMinter.sol

79       require(sfrxeth_recieved > 0, 'No sfrxETH was returned');
87       require(!submitPaused, "Submit is paused");
88       require(msg.value != 0, "Cannot submit 0");
122      require(!depositEtherPaused, "Depositing ETH is paused");
126      require(numDeposits > 0, "Not enough ETH in contract");
140      require(!activeValidators[pubKey], "Validator already has 32 ETH");
160      require (newRatio <= RATIO_PRECISION, "Ratio cannot surpass 100%");
167      require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");
193      require(success, "Invalid transfer");

File: src/OperatorRegistry.sol

46       require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock"); 
137      require(numVals != 0, "Validator stack is empty");
182      require(numValidators() == 0, "Clear validator array first");
203      require(_timelock_address != address(0), "Zero address detected");

7- It costs more gas to initialize variables to zero than to let the default of zero be applied (saves ~3 gas per instance) :

If a variable is not set/initialized, it is assumed to have the default value (0 for uint or int, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

There are 8 instances of this issue:

File: src/ERC20/ERC20PermitPermissionedMint.sol Line 84

for (uint i = 0; i < minters_array.length; i++)

File: src/OperatorRegistry.sol Line 63

for (uint256 i = 0; i < arrayLength; ++i)

File: src/OperatorRegistry.sol Line 84

for (uint256 i = 0; i < times; ++i)

File: src/OperatorRegistry.sol Line 114

for (uint256 i = 0; i < original_validators.length; ++i)

File: src/frxETHMinter.sol Line 63

withholdRatio = 0;

File: src/frxETHMinter.sol Line 64

currentWithheldETH = 0;

File: src/frxETHMinter.sol Line 94

uint256 withheld_amt = 0;

File: src/frxETHMinter.sol Line 129

for (uint256 i = 0; i < numDeposits; ++i)

8- Use of ++i cost less gas than i++/i=i+1 in for loops :

There is 1 instance of this issue:

File: src/ERC20/ERC20PermitPermissionedMint.sol Line 84

for (uint i = 0; i < minters_array.length; i++)

9- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 5 instances of this issue:

File: src/ERC20/ERC20PermitPermissionedMint.sol Line 84

for (uint i = 0; i < minters_array.length; i++)

File: src/frxETHMinter.sol Line 129

for (uint256 i = 0; i < numDeposits; ++i)

File: src/OperatorRegistry.sol Line 63

for (uint256 i = 0; i < arrayLength; ++i)

File: src/OperatorRegistry.sol Line 84

for (uint256 i = 0; i < times; ++i)

File: src/OperatorRegistry.sol Line 114

for (uint256 i = 0; i < original_validators.length; ++i)

10- Using private rather than public for constants, saves gas :

If needed, the value can be read from the verified contract source code. Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

There are 2 instances of this issue:

File: src/frxETHMinter.sol Line 38

uint256 public constant DEPOSIT_SIZE = 32 ether;

File: src/frxETHMinter.sol Line 39

uint256 public constant RATIO_PRECISION = 1e6;

11- Functions guaranteed to revert when called by normal users can be marked payable :

If a function modifier such as onlyAdmin is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for the owner because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are :

CALLVALUE(gas=2), DUP1(gas=3), ISZERO(gas=3), PUSH2(gas=3), JUMPI(gas=10), PUSH1(gas=3), DUP1(gas=3), REVERT(gas=0), JUMPDEST(gas=1), POP(gas=2). Which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 17 instances of this issue:

File: src/ERC20/ERC20PermitPermissionedMint.sol

65       function addMinter(address minter_address) public onlyByOwnGov
76       function removeMinter(address minter_address) public onlyByOwnGov
94       function setTimelock(address _timelock_address) public onlyByOwnGov

File: src/frxETHMinter.sol

159      function setWithholdRatio(uint256 newRatio) external onlyByOwnGov
166      function moveWithheldETH(address payable to, uint256 amount) external onlyByOwnGov
177      function togglePauseSubmits() external onlyByOwnGov
184      function togglePauseDepositEther() external onlyByOwnGov
191      function recoverEther(uint256 amount) external onlyByOwnGov
199      function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyByOwnGov

File: src/OperatorRegistry.sol

53       function addValidator(Validator calldata validator) public onlyByOwnGov
61       function addValidators(Validator[] calldata validatorArray) external onlyByOwnGov
69       function swapValidator(uint256 from_idx, uint256 to_idx) public onlyByOwnGov
82       function popValidators(uint256 times) public onlyByOwnGov
93       function removeValidator(uint256 remove_idx, bool dont_care_about_ordering) public onlyByOwnGov
181      function setWithdrawalCredential(bytes memory _new_withdrawal_pubkey) external onlyByOwnGov
190      function clearValidatorArray() external onlyByOwnGov
202      function setTimelock(address _timelock_address) external onlyByOwnGov