Frax Ether Liquid Staking contest - TomJ's results

Lines of code

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

Vulnerability details

Impact

A malicious timelock_address and owner can withdraw all of the ETH submitted to frxETHMinter contract at anytime through recoverEther function. Rug pull vector available in the contract might negatively impact the protocol's reputation.

Proof of Concept

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

        emit EmergencyEtherRecovered(amount);
    }

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider removing the function or settting timelock_address and owner addresses as timelock DAO or multisig.

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L120-L155

Vulnerability details

Impact

There are no upper bounds on how many times depositEther function of frxETHMinter contract will make a deposit. Since gas requirements can change over time, if frxETHMinter contract receive enormous amount of ETH in a short time, it may cause the function to fail due to out of gas error.

Therefore, frxETHMinter contract might end up not being able to execute depositEther forever unless admin temporary withdraw some of ETH out of the contract.

Proof of Concept

No upper bounds set on how many times it will deposit.

        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
            );

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add upper bound on how much depositEther function will deposit at once or add function that will deposit only once at each transaction.

Table of Contents

Low Risk Issues

• Missing Zero Address Check

Non-critical Issues

• Use fixed compiler versions instead of floating version
• Events Not Emitted for Important State Changes
• Event is Missing Indexed Fields

 

Low Risk Issues

Missing Zero Address Check

Issue

I recommend adding check of 0-address for input validation of critical address parameters. Not doing so might lead to non-functional contract and have to redeploy the contract, when it is updated to 0-address accidentally. For functions other than constructor(), lack of 0-address check might cause loss of funds for the user.

PoC

Total of 9 instances found.

1. ERC20PermitPermissionedMint.sol:constructor(): "timelock_address" address https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/ERC20/ERC20PermitPermissionedMint.sol#L34

2. OperatorRegistry.sol:constructor(): "timelock_address" address https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L41

3. frxETHMinter.sol:constructor(): "depositContract" address https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L60

4. frxETHMinter.sol:constructor(): "frxETHAddress" address https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L61

5. frxETHMinter.sol:constructor(): "sfrxETHToken" address https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L62

6. frxETHMinter.sol:submitAndDeposit(): "recipient" address https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L70

7. frxETHMinter.sol:moveWithheldETH(): "to" address https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L170

8. sfrxETH.sol:depositWithSignature(): "receiver" address https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L70

9. sfrxETH.sol:mintWithSignature(): "receiver" address https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L86

Mitigation

Add 0-address check for above addresses.

 

Non-critical Issues

Use fixed compiler versions instead of floating version

Issue

it is best practice to lock your pragma instead of using floating pragma. the use of floating pragma has a risk of accidentally get deployed using latest complier which may have higher risk of undiscovered bugs. Reference: https://consensys.github.io/smart-contract-best-practices/development-recommendations/solidity-specific/locking-pragmas/

PoC

Total of 6 instances found.

./sfrxETH.sol:2:pragma solidity ^0.8.0;
./ERC20PermitPermissionedMint.sol:2:pragma solidity ^0.8.0;
./frxETHMinter.sol:2:pragma solidity ^0.8.0;
./OperatorRegistry.sol:2:pragma solidity ^0.8.0;
./frxETH.sol:2:pragma solidity ^0.8.0;
./xERC4626.sol:4:pragma solidity ^0.8.0;

Mitigation

I suggest to lock your pragma and aviod using floating pragma.

// bad
pragma solidity ^0.8.10;

// good
pragma solidity 0.8.10;

 

Events Not Emitted for Important State Changes

Issue

It is best practice to emit an event when we there is important state changes like update a dynamic array or mapping because it allows monitoring activities with off-chain monitoring tools.

PoC

Total of 1 instance found.

1. OperatorRegistry.sol:getNextValidator(): No event is emitted even though "validators" array is modified. https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L141

Mitigation

Emit an event when there is important state changes.

 

Event is Missing Indexed Fields

Issue

Each event should have 3 indexed fields if there are 3 or more fields.

PoC

Total of 19 instances found.

./ERC20PermitPermissionedMint.sol:102:    event TokenMinterBurned(address indexed from, address indexed to, uint256 amount);
./ERC20PermitPermissionedMint.sol:103:    event TokenMinterMinted(address indexed from, address indexed to, uint256 amount);
./ERC20PermitPermissionedMint.sol:104:    event MinterAdded(address minter_address);
./ERC20PermitPermissionedMint.sol:105:    event MinterRemoved(address minter_address);
./ERC20PermitPermissionedMint.sol:106:    event TimelockChanged(address timelock_address);
./frxETHMinter.sol:205:    event EmergencyEtherRecovered(uint256 amount);
./frxETHMinter.sol:206:    event EmergencyERC20Recovered(address tokenAddress, uint256 tokenAmount);
./frxETHMinter.sol:207:    event ETHSubmitted(address indexed sender, address indexed recipient, uint256 sent_amount, uint256 withheld_amt);
./frxETHMinter.sol:208:    event DepositEtherPaused(bool new_status);
./frxETHMinter.sol:209:    event DepositSent(bytes indexed pubKey, bytes withdrawalCredential);
./frxETHMinter.sol:210:    event SubmitPaused(bool new_status);
./frxETHMinter.sol:211:    event WithheldETHMoved(address indexed to, uint256 amount);
./frxETHMinter.sol:212:    event WithholdRatioSet(uint256 newRatio);
./OperatorRegistry.sol:208:    event TimelockChanged(address timelock_address);
./OperatorRegistry.sol:209:    event WithdrawalCredentialSet(bytes _withdrawalCredential);
./OperatorRegistry.sol:210:    event ValidatorAdded(bytes pubKey, bytes withdrawalCredential);
./OperatorRegistry.sol:212:    event ValidatorRemoved(bytes pubKey, uint256 remove_idx, bool dont_care_about_ordering);
./OperatorRegistry.sol:213:    event ValidatorsPopped(uint256 times);
./OperatorRegistry.sol:214:    event ValidatorsSwapped(bytes from_pubKey, bytes to_pubKey, uint256 from_idx, uint256 to_idx);

Mitigation

Add up to 3 indexed fields when possible.

 

Table of Contents

Total of 11 issues found

• X = X + Y costs less gass than X += Y
• Duplicate require() Checks Should be a Modifier or a Function
• Change Function Visibility Public to External
• Use Calldata instead of Memory for Read Only Function Parameters
• Boolean Comparisons
• Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas
• Store Array's Length as a Variable
• ++i Costs Less Gas than i++
• != 0 costs less gass than > 0
• Keep The Revert Strings of Error Messages within 32 Bytes
• Use Custom Errors to Save Gas

 

X = X + Y costs less gass than X += Y

Issue

X = X + Y costs less gass than X += Y for state variables. It saves 8 gas.

PoC

Total of 2 instance found.

1. moveWithheldETH function of frxETHMinter.sol https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L168 Change it to

currentWithheldETH = currentWithheldETH - amount;

1. _submit function of frxETHMinter.sol https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L97 Change it to

currentWithheldETH = currentWithheldETH + withheld_amt;

 

Duplicate require() Checks Should be a Modifier or a Function

Issue

Since below require checks are used more than once, I recommend making these to a modifier or a function.

PoC

Total of 2 instances found.

./ERC20PermitPermissionedMint.sol:66:        require(minter_address != address(0), "Zero address detected");
./ERC20PermitPermissionedMint.sol:77:        require(minter_address != address(0), "Zero address detected");

./frxETHMinter.sol:171:        require(success, "Invalid transfer");
./frxETHMinter.sol:193:        require(success, "Invalid transfer");

Mitigation

I recommend making duplicate require statement into modifier or a function.

 

Change Function Visibility Public to External

Issue

If the function is not called internally, it is cheaper to set your function visibility to external instead of public.

PoC

Total of 3 instances found.

1. sfrxETH.sol:pricePerShare() https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L54-L56

2. OperatorRegistry.sol:popValidators() https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L82-L89

3. OperatorRegistry.sol:removeValidators() https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/OperatorRegistry.sol#L93-L122

Mitigation

Change the function visibility to external.

 

Use Calldata instead of Memory for Read Only Function Parameters

Issue

It is cheaper gas to use calldata than memory if the function parameter is read only. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory. More details on following link. link: https://docs.soliditylang.org/en/v0.8.15/types.html#data-location

PoC

Total of 3 instances found.

./OperatorRegistry.sol:172:        bytes memory pubKey, 
./OperatorRegistry.sol:173:        bytes memory signature, 
./OperatorRegistry.sol:181:    function setWithdrawalCredential(bytes memory _new_withdrawal_pubkey) external onlyByOwnGov {

Mitigation

Change memory to calldata

 

Boolean Comparisons

Issue

It is more gas expensive to compare boolean with "variable == true" or "variable == false" than directly checking the returned boolean value.

PoC

Total of 3 instances found.

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

Mitigation

Simply check by returned boolean value. Example:

For false:
require(!somethinghere);
For true:
require(somethinghere);

 

Using Elements Smaller than 32 bytes (256 bits) Might Use More Gas

Issue

Since EVM operates on 32 bytes at a time, if the element is smaller than that, the EVM must use more operations in order to reduce the elements from 32 bytes to specified size.

Reference: https://docs.soliditylang.org/en/v0.8.15/internals/layout_in_storage.html

PoC

Total of 8 instances found.

./sfrxETH.sol:64:        uint8 v,
./sfrxETH.sol:80:        uint8 v,
./xERC4626.sol:48:        uint192 lastRewardAmount_ = lastRewardAmount;
./xERC4626.sol:49:        uint32 rewardsCycleEnd_ = rewardsCycleEnd;
./xERC4626.sol:50:        uint32 lastSync_ = lastSync;
./xERC4626.sol:79:        uint192 lastRewardAmount_ = lastRewardAmount;
./xERC4626.sol:80:        uint32 timestamp = block.timestamp.safeCastTo32();
./xERC4626.sol:89:        uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;

Mitigation

I suggest using uint256 instead of anything smaller and downcast where needed.

 

Store Array's Length as a Variable

Issue

By storing an array's length as a variable before the for-loop, can save 3 gas per iteration.

PoC

Total of 2 instances found.

./ERC20PermitPermissionedMint.sol:84:        for (uint i = 0; i < minters_array.length; i++){ 
./OperatorRegistry.sol:114:            for (uint256 i = 0; i < original_validators.length; ++i) {

Mitigation

Store array's length as a variable before looping it. For example, I suggest changing it to

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

 

++i Costs Less Gas than i++

Issue

Prefix increments/decrements (++i or --i) costs cheaper gas than postfix increment/decrements (i++ or i--).

PoC

Total of 1 instance found.

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

Mitigation

Change it to postfix increments/decrements. It saves 6 gas per loop.

 

!= 0 costs less gass than > 0

Issue

!= 0 costs less gas when optimizer is enabled and is used for unsigned integers in require statement.

PoC

Total of 2 instances found.

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

Mitigation

I suggest changing > 0 to != 0

 

Keep The Revert Strings of Error Messages within 32 Bytes

Issue

Since each storage slot is size of 32 bytes, error messages that is longer than this will need extra storage slot leading to more gas cost.

PoC

Total of 1 instance found.

./frxETHMinter.sol:167:        require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");

Mitigation

Simply keep the error messages within 32 bytes to avoid extra storage slot cost.

 

Use Custom Errors to Save Gas

Issue

Custom errors from Solidity 0.8.4 are cheaper than revert strings. Details are explained here: https://blog.soliditylang.org/2021/04/21/custom-errors/

PoC

Total of 22 instances found.

./ERC20PermitPermissionedMint.sol:41:        require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");
./ERC20PermitPermissionedMint.sol:46:       require(minters[msg.sender] == true, "Only minters");
./ERC20PermitPermissionedMint.sol:66:        require(minter_address != address(0), "Zero address detected");
./ERC20PermitPermissionedMint.sol:68:        require(minters[minter_address] == false, "Address already exists");
./ERC20PermitPermissionedMint.sol:77:        require(minter_address != address(0), "Zero address detected");
./ERC20PermitPermissionedMint.sol:78:        require(minters[minter_address] == true, "Address nonexistant");
./ERC20PermitPermissionedMint.sol:95:        require(_timelock_address != address(0), "Zero address detected"); 
./frxETHMinter.sol:79:        require(sfrxeth_recieved > 0, 'No sfrxETH was returned');
./frxETHMinter.sol:87:        require(!submitPaused, "Submit is paused");
./frxETHMinter.sol:88:        require(msg.value != 0, "Cannot submit 0");
./frxETHMinter.sol:122:        require(!depositEtherPaused, "Depositing ETH is paused");
./frxETHMinter.sol:126:        require(numDeposits > 0, "Not enough ETH in contract");
./frxETHMinter.sol:140:            require(!activeValidators[pubKey], "Validator already has 32 ETH");
./frxETHMinter.sol:160:        require (newRatio <= RATIO_PRECISION, "Ratio cannot surpass 100%");
./frxETHMinter.sol:167:        require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");
./frxETHMinter.sol:171:        require(success, "Invalid transfer");
./frxETHMinter.sol:193:        require(success, "Invalid transfer");
./frxETHMinter.sol:200:        require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");
./OperatorRegistry.sol:46:        require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");
./OperatorRegistry.sol:137:        require(numVals != 0, "Validator stack is empty");
./OperatorRegistry.sol:182:        require(numValidators() == 0, "Clear validator array first");
./OperatorRegistry.sol:203:        require(_timelock_address != address(0), "Zero address detected");

Mitigation

I suggest implementing custom errors to save gas.