Frax Ether Liquid Staking contest - brgltd's results

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L199-L203

Vulnerability details

It will not be possible to recover all ERC20 tokens sent to the contract

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L199-L203

Impact

Not all tokens follow the ERC20 standard. E.g. USDT will not return boolean on transfer.

Even if the protocol does not plan to add USDT integration, it's possible to USDT to be sent accidentaly, in which case the function recoverERC20 will fail due to abnormal ERC20 token behaviour, which will prevent the tokens from being recovered.

function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyByOwnGov {
    require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");

    emit EmergencyERC20Recovered(tokenAddress, tokenAmount);
}

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L199-L203

Recommended Mitigation Steps

Replace IERC20 with a SafeERC20 implementation. OpenZeppelin and Solmate are the most populars. With this change, it would be possible to recover ERC20 tokens even without boolean returns.

[NC-01] Public functions not called by the contract can be refactored to be external functions

function popValidators(uint256 times) public onlyByOwnGov {

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L82

function removeValidator(uint256 remove_idx, bool dont_care_about_ordering) public onlyByOwnGov {

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L93

[NC-02] Lack of checks-effects-interactions

function depositEther() external nonReentrant {
    // Initial pause check
    require(!depositEtherPaused, "Depositing ETH is paused");

    // See how many deposits can be made. Truncation desired.
    uint256 numDeposits = (address(this).balance - currentWithheldETH) / DEPOSIT_SIZE;
    require(numDeposits > 0, "Not enough ETH in contract");

    // Give each deposit chunk to an empty validator
    for (uint256 i = 0; i < numDeposits; ++i) {
        // Get validator information
        (
            bytes memory pubKey,
            bytes memory withdrawalCredential,
            bytes memory signature,
            bytes32 depositDataRoot
        ) = getNextValidator(); // Will revert if there are not enough free validators

        // Make sure the validator hasn't been deposited into already, to prevent stranding an extra 32 eth
        // until withdrawals are allowed
        require(!activeValidators[pubKey], "Validator already has 32 ETH");

        // Deposit the ether in the ETH 2.0 deposit contract
        depositContract.deposit{value: DEPOSIT_SIZE}(
            pubKey,
            withdrawalCredential,
            signature,
            depositDataRoot
        );

        // Set the validator as used so it won't get an extra 32 ETH
        activeValidators[pubKey] = true;

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L120-L151

The depositEther() function is updating the state variable activeValidators before the depositContract.deposit() call. The function is not at risk of reentrancy due to the nonReentrant modifier. However, to follow the checks-effects-interactions, state modifications should be computed before the external call to ensure the state is only updated on successul calls.

In this case, the following modification could be made.

$ git diff --no-index frxETHMinter.sol.orig frxETHMinter.sol
diff --git a/frxETHMinter.sol.orig b/frxETHMinter.sol
index a33bdbe..6f5fd2e 100644
--- a/frxETHMinter.sol.orig
+++ b/frxETHMinter.sol
@@ -139,6 +139,9 @@ contract frxETHMinter is OperatorRegistry, ReentrancyGuard {
             // until withdrawals are allowed
             require(!activeValidators[pubKey], "Validator already has 32 ETH");

+            // Set the validator as used so it won't get an extra 32 ETH
+            activeValidators[pubKey] = true;
+
             // Deposit the ether in the ETH 2.0 deposit contract
             depositContract.deposit{value: DEPOSIT_SIZE}(
                 pubKey,
@@ -147,9 +150,6 @@ contract frxETHMinter is OperatorRegistry, ReentrancyGuard {
                 depositDataRoot
             );

-            // Set the validator as used so it won't get an extra 32 ETH
-            activeValidators[pubKey] = true;
-
             emit DepositSent(pubKey, withdrawalCredential);
         }
     }

[NC-03] Normalize between snake_case and mixedCase

Some arguments and variables are written in snake_case

function removeValidator(uint256 remove_idx, bool dont_care_about_ordering) public onlyByOwnGov {

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L93

bytes memory removed_pubkey = validators[remove_idx].pubKey;

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L95

While others are mixedCase.

function getValidatorStruct(
    bytes memory pubKey,
    bytes memory signature,
    bytes32 depositDataRoot
) external pure returns (Validator memory) {

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L171-L175

uint arrayLength = validatorArray.length;

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L62

Consider using only one convention. Note that the solidity docs recommends mixedCase for both function arguments and local variables.

[NC-04] The algorithm to remove a validator while preserving the order can be improved

The current algorithm will create a new array and push every element except the one to be excluded.

// 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]);
    }
}

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L107-L118

A better approach would be to switch elements (i with i + 1) starting from the remove_idx until the last index, and then pop the last element. This would also improve performance and gas consumption if the target element is located at the end of the array (since it won't traverse the entire array anymore).

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

[NC-05] Missing zero-address checks in constructors

Missing checks for zero-addresses may lead to infunctional protocol, if the variable addresses are initialized incorrectly.

constructor(
    address depositContractAddress,
    address frxETHAddress,
    address sfrxETHAddress,
    address _owner,
    address _timelock_address,

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L52-L57

constructor(
    address _creator_address,
    address _timelock_address,

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L24-L26

[NC-06] Non library/interface files should use fixed compiler verion

Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version.

Note that 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 a package.

The following 5 contracts are not libraries and are floating the pragma.

src/frxETH.sol,
src/sfrxETH.sol,
src/ERC20/ERC20PermitPermissionedMint.sol,
src/frxETHMinter.sol,
src/OperatorRegistry.sol,

[NC-07] block.timestamp should not be downcasted to 32 bits

32-bits will stop working after January 19th 2038 for storing unix timestamp. This is likely not a real issue for the protocol, since 16 years is still a lot of time. However, from a purely software resilience and quality assurance point of view, I would recommend to use 40 bits when downcasting block.timestamp.

[NC-08] Modify the order of functions and events

The official solidity style guide recommends to declare an event before functions. Also, another recommendation is to declare public functions bellow external functions.

/// @notice Empties the validator array
/// @dev Need to do this before setWithdrawalCredential()
function clearValidatorArray() external onlyByOwnGov {
    delete validators;

    emit ValidatorArrayCleared();
}

/// @notice Returns the number of validators
function numValidators() public view returns (uint256) {
    return validators.length;
}

/// @notice Set the timelock contract
function setTimelock(address _timelock_address) external onlyByOwnGov {
    require(_timelock_address != address(0), "Zero address detected");
    timelock_address = _timelock_address;
    emit TimelockChanged(_timelock_address);
}

event TimelockChanged(address timelock_address);
event WithdrawalCredentialSet(bytes _withdrawalCredential);
event ValidatorAdded(bytes pubKey, bytes withdrawalCredential);
event ValidatorArrayCleared();
event ValidatorRemoved(bytes pubKey, uint256 remove_idx, bool dont_care_about_ordering);
event ValidatorsPopped(uint256 times);
event ValidatorsSwapped(bytes from_pubKey, bytes to_pubKey, uint256 from_idx, uint256 to_idx);
event KeysCleared();

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L188-L215

[NC-09] Critical changes should use two-step procedure

Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.

function setTimelock(address _timelock_address) public onlyByOwnGov {

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol#L94

function setWithholdRatio(uint256 newRatio) external onlyByOwnGov {

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol

function setWithdrawalCredential(bytes memory _new_withdrawal_pubkey) external onlyByOwnGov {

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L181

function setTimelock(address _timelock_address) external onlyByOwnGov {

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L202

[NC-10] Inconsistent use of return named variable

Consider removing the shares state variable or refactoring submitAndDeposit() to use the return named variable.

function submitAndDeposit(address recipient) external payable returns (uint256 shares) {
    // Give the frxETH to this contract after it is generated
    _submit(address(this));

    // Approve frxETH to sfrxETH for staking
    frxETHToken.approve(address(sfrxETHToken), msg.value);

    // Deposit the frxETH and give the generated sfrxETH to the final recipient
    uint256 sfrxeth_recieved = sfrxETHToken.deposit(msg.value, recipient);
    require(sfrxeth_recieved > 0, 'No sfrxETH was returned');

    return sfrxeth_recieved;

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L70-L82

[G-01] Prefix increment costs less gas than postfix increment

There's 1 instance of this issue.

File: src/ERC20/ERC20PermitPermissionedMint.sol
84: for (uint i = 0; i < minters_array.length; i++){

[G-02] The increment in for loop post condition can be made unchecked to save gas

There are 5 instances of this issue.

File: src/ERC20/ERC20PermitPermissionedMint.sol
84: for (uint i = 0; i < minters_array.length; i++){

File: src/OperatorRegistry.sol
63: for (uint256 i = 0; i < arrayLength; ++i) {
84: for (uint256 i = 0; i < times; ++i) {
114: for (uint256 i = 0; i < original_validators.length; ++i) {

File: src/frxETHMinter.sol
129: for (uint256 i = 0; i < numDeposits; ++i) {

[G-03] Cache the length of the array before the loop

There are 2 instances of this issue.

File: src/ERC20/ERC20PermitPermissionedMint.sol
84: for (uint i = 0; i < minters_array.length; i++){

File: src/OperatorRegistry.sol
114: for (uint256 i = 0; i < original_validators.length; ++i) {

[G-04] Initializing a variable with the default value wastes gas

There are 5 instances of this issue.

File: src/ERC20/ERC20PermitPermissionedMint.sol
84: for (uint i = 0; i < minters_array.length; i++){

File: src/OperatorRegistry.sol
63: for (uint256 i = 0; i < arrayLength; ++i) {
84: for (uint256 i = 0; i < times; ++i) {
114: for (uint256 i = 0; i < original_validators.length; ++i) {

File: src/frxETHMinter.sol
129: for (uint256 i = 0; i < numDeposits; ++i) {

[G-05] Use != 0 instead of > 0 to save gas

Replace > 0 with != 0 for unsigned integers.

There are 2 instances of this issue.

File: src/frxETHMinter.sol
79: require(sfrxeth_recieved > 0, 'No sfrxETH was returned');
126: require(numDeposits > 0, "Not enough ETH in contract");

[G-06] Use custom errors rather than require/revert strings to save gas

There are 21 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/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");

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");
167: require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");
171: require(success, "Invalid transfer");
193: require(success, "Invalid transfer");
200: require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");

[G-07] Don’t compare boolean expressions to boolean literals

There are 3 instances of this issue.

File: src/ERC20/ERC20PermitPermissionedMint.sol
46: require(minters[msg.sender] == true, "Only minters");
68: require(minters[minter_address] == false, "Address already exists");
78: require(minters[minter_address] == true, "Address nonexistant");

[G-08] Using private rather than public for constants, saves gas

The values can still be inspected on the source code if necessary.

There are 2 instances of this issue.

File: src/frxETHMinter.sol
38: uint256 public constant DEPOSIT_SIZE = 32 ether; // ETH 2.0 minimum deposit size
39: uint256 public constant RATIO_PRECISION = 1e6; // 1,000,000

[G-09] x += y costs more gas than x = x + y for state variables

There are 4 instances of this issue.

File: lib/ERC4626/src/xERC4626.sol
67: storedTotalAssets -= amount;
72: storedTotalAssets += amount;

File: src/frxETHMinter.sol
97: currentWithheldETH += withheld_amt;
168: currentWithheldETH -= amount;