Frax Ether Liquid Staking contest - rotcivegaf's results

Lines of code

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

Vulnerability details

Impact

Could be the case where the validators available are lower than the total deposit chunks and when try use the depositEther() function won't be able to deposit into the depositContract because the getNextValidator() reverts when the validators array it's empty For example: We have 2 validators available and 32 * 4 ETH balance in frxETHMinter The depositEther() try create 4 deposits but only have validators for 2 and in the third cycle it broke

Proof of Concept

) = getNextValidator(); // Will revert if there are not enough free validators

require(numVals != 0, "Validator stack is empty");

Tools Used

Review

Recommended Mitigation Steps

Send the numDeposits as a parameter to control the deposits

    /// @notice Deposit batches of ETH to the ETH 2.0 deposit contract
    /// @dev Usually a bot will call this periodically
    function depositEther(uint256 numDeposits) external nonReentrant {
        // Initial pause check
        require(!depositEtherPaused, "Depositing ETH is paused");

        if (numDeposits == 0) {
            // See how many deposits can be made. Truncation desired.
            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;

            emit DepositSent(pubKey, withdrawalCredential);
        }
    }

Lines of code

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L43-L62 https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/sfrxETH.sol#L73-L87

Vulnerability details

Impact

To use mintWithSignature function need make a signature and send the transaction There are a window time since the signer make the signature, send the mintWithSignature transaction and this one it's confirmed In this window time the previewMint calculation could be change because the previewMint calculation use totalSupply and totalAssets in his equation and this variables are changed everytime with an deposit, mint, withdraw, etc The signer was signed the permit with an old amount and this one change, invalidating the signature, and revert the transaction

Proof of Concept

uint256 amount = approveMax ? type(uint256).max : previewMint(shares);

Tools Used

Review

Recommended Mitigation Steps

• Replace the approveMax logic for a assetsApproved
• The assetsApproved was the amount of assets who the contract has the permit
• Calculate the assetsApproved with the previewMint function offchain and permit an extra percentage amount

    function mintWithSignature(
        uint256 assetsApproved,
        uint256 shares,
        address receiver,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external nonReentrant returns (uint256 assets) {
        asset.permit(msg.sender, address(this), assetsApproved, deadline, v, r, s);
        return (mint(shares, receiver));
    }

QA report

Low Risk

Total: 12 instances over 3 issues

Non-critical

Total: 72 instances over 7 issues

Low Risk

[L-01] Missing zero address checks

File: /src/ERC20/ERC20PermitPermissionedMint.sol

26        address _timelock_address,

File: /src/sfrxETH.sol

42    constructor(ERC20 _underlying, uint32 _rewardsCycleLength)

File: /src/frxETHMinter.sol

 53        address depositContractAddress,

 54        address frxETHAddress,

 55        address sfrxETHAddress,

 57        address _timelock_address,

 70    function submitAndDeposit(address recipient) external payable returns (uint256 shares) {

166    function moveWithheldETH(address payable to, uint256 amount) external onlyByOwnGov {

File: /src/OperatorRegistry.sol

/*_timelock_address parameter*/
40     constructor(address _owner, address _timelock_address, bytes memory _withdrawal_pubkey) Owned(_owner) {

[L-02] Draft OpenZeppelin Dependencies

The ERC20PermitPermissionedMint.sol contract heredit from an OpenZeppelin contract who is still a draft and is not considered ready for mainnet use. OpenZeppelin contracts may be considered draft contracts if they have not received adequate security auditing or are liable to change with future development.

Recommendation

Ensure the development team is aware of the risks of using a draft contract or consider waiting until the contract is finalised.

Otherwise, make sure that development team are aware of the risks of using a draft OpenZeppelin contract and accept the risk-benefit trade-off.

Also could evaluate changing to the solmate contracts since his ERC20 implementation already has the EIP-2612 permit

File: /src/ERC20/ERC20PermitPermissionedMint.sol

6 import "openzeppelin-contracts/contracts/token/ERC20/extensions/draft-ERC20Permit.sol";

[L-03] Don't use owner and timelock

Using a timelock contract gives confidence to the user, but when check onlyByOwnGov allow the owner and the timelock The owner manipulate the contract without a lock time period

Recommendation

• Use only Owned permission
• Remove the timelock_address
• The owner should be the timelock contract

File: /src/frxETH.sol

38      address _timelock_address

40    ERC20PermitPermissionedMint(_creator_address, _timelock_address, "Frax Ether", "frxETH")

File: /src/ERC20/ERC20PermitPermissionedMint.sol

 16    address public timelock_address;

 26        address _timelock_address,

 34      timelock_address = _timelock_address;

 41        require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");

 94    function setTimelock(address _timelock_address) public onlyByOwnGov {
 95        require(_timelock_address != address(0), "Zero address detected");
 96        timelock_address = _timelock_address;
 97        emit TimelockChanged(_timelock_address);
 98    }

106    event TimelockChanged(address timelock_address);

File: /src/frxETH.sol

38      address _timelock_address

40    ERC20PermitPermissionedMint(_creator_address, _timelock_address, "Frax Ether", "frxETH")

File: /src/OperatorRegistry.sol

 38    address public timelock_address;

 40    constructor(address _owner, address _timelock_address, bytes memory _withdrawal_pubkey) Owned(_owner) {
 41        timelock_address = _timelock_address;

 46        require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");

202    function setTimelock(address _timelock_address) external onlyByOwnGov {
203        require(_timelock_address != address(0), "Zero address detected");
204        timelock_address = _timelock_address;
205        emit TimelockChanged(_timelock_address);
206    }

208    event TimelockChanged(address timelock_address);

File: /src/frxETHMinter.sol

57        address _timelock_address,

59    ) OperatorRegistry(_owner, _timelock_address, _withdrawalCredential) {

Non-critical

[N-01] Unused imports

File: /src/ERC20/ERC20PermitPermissionedMint.sol

4 import "openzeppelin-contracts/contracts/token/ERC20/ERC20.sol";

5 import "openzeppelin-contracts/contracts/token/ERC20/IERC20.sol";

[N‑02] Non-library/interface files should use fixed compiler versions, not floating ones

File: /src/ERC20/ERC20PermitPermissionedMint.sol

2 pragma solidity ^0.8.0;

File: /src/frxETH.sol

2 pragma solidity ^0.8.0;

File: /src/sfrxETH.sol

2 pragma solidity ^0.8.0;

File: /src/frxETHMinter.sol

2 pragma solidity ^0.8.0;

File: /src/OperatorRegistry.sol

2 pragma solidity ^0.8.0;

File: /src/xERC4626.sol

4 pragma solidity ^0.8.0;

[N‑03] Lint

Wrong identation:

File: /src/ERC20/ERC20PermitPermissionedMint.sol

From:
30    ERC20(_name, _symbol)
31    ERC20Permit(_name)
32    Owned(_creator_address)
To:
30        ERC20(_name, _symbol)
31        ERC20Permit(_name)
32        Owned(_creator_address)

File: /src/frxETH.sol

From:
37      address _creator_address,
38      address _timelock_address
To:
37        address _creator_address,
38        address _timelock_address

From:
40    ERC20PermitPermissionedMint(_creator_address, _timelock_address, "Frax Ether", "frxETH")
To:
40        ERC20PermitPermissionedMint(_creator_address, _timelock_address, "Frax Ether", "frxETH")

Don't use extra parenthesis:

File: /src/sfrxETH.sol

70        return (deposit(assets, receiver));

86        return (mint(shares, receiver));

Miss space:

File: /src/ERC20/ERC20PermitPermissionedMint.sol

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

Remove space:

File: /src/ERC20/ERC20PermitPermissionedMint.sol

63 \n

File: /src/frxETH.sol

34 \n

42 \n

File: /src/sfrxETH.sol

88 \n

File: /src/OperatorRegistry.sol

29 \n

[N‑04] Event is missing indexed fields

Index event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.

File: /src/frxETHMinter.sol

205    event EmergencyEtherRecovered(uint256 amount);

206    event EmergencyERC20Recovered(address tokenAddress, uint256 tokenAmount);

207    event ETHSubmitted(address indexed sender, address indexed recipient, uint256 sent_amount, uint256 withheld_amt);

208    event DepositEtherPaused(bool new_status);

209    event DepositSent(bytes indexed pubKey, bytes withdrawalCredential);

210    event SubmitPaused(bool new_status);

211    event WithheldETHMoved(address indexed to, uint256 amount);

212    event WithholdRatioSet(uint256 newRatio);

File: /src/OperatorRegistry.sol

208    event TimelockChanged(address timelock_address);

209    event WithdrawalCredentialSet(bytes _withdrawalCredential);

210    event ValidatorAdded(bytes pubKey, bytes withdrawalCredential);

212    event ValidatorRemoved(bytes pubKey, uint256 remove_idx, bool dont_care_about_ordering);

213    event ValidatorsPopped(uint256 times);

214    event ValidatorsSwapped(bytes from_pubKey, bytes to_pubKey, uint256 from_idx, uint256 to_idx);

File: /src/ERC20/ERC20PermitPermissionedMint.sol

102    event TokenMinterBurned(address indexed from, address indexed to, uint256 amount);

103    event TokenMinterMinted(address indexed from, address indexed to, uint256 amount);

104    event MinterAdded(address minter_address);

105    event MinterRemoved(address minter_address);

106    event TimelockChanged(address timelock_address);

[N-05] Functions, parameters and variables in snake case

Use camel case for all functions, parameters and variables and snake case for constants

File: /src/ERC20/ERC20PermitPermissionedMint.sol

 16    address public timelock_address;

 19    address[] public minters_array; // Allowed to mint

 25        address _creator_address,

 26        address _timelock_address,

 53    function minter_burn_from(address b_address, uint256 b_amount) public onlyMinters {

 59    function minter_mint(address m_address, uint256 m_amount) public onlyMinters {

 65    function addMinter(address minter_address) public onlyByOwnGov {

 76    function removeMinter(address minter_address) public onlyByOwnGov {

 94    function setTimelock(address _timelock_address) public onlyByOwnGov {

104    event MinterAdded(address minter_address);

105    event MinterRemoved(address minter_address);

106    event TimelockChanged(address timelock_address);

File: /src/frxETH.sol

37      address _creator_address,

38      address _timelock_address

File: /src/frxETHMinter.sol

 57        address _timelock_address,

 78        uint256 sfrxeth_recieved = sfrxETHToken.deposit(msg.value, recipient);

 94        uint256 withheld_amt = 0;

208    event DepositEtherPaused(bool new_status);

210    event SubmitPaused(bool new_status);

File: /src/OperatorRegistry.sol

 37    bytes curr_withdrawal_pubkey; // Pubkey for ETH 2.0 withdrawal creds. If you change it, you must empty the validators array

 38    address public timelock_address;

 40    constructor(address _owner, address _timelock_address, bytes memory _withdrawal_pubkey) Owned(_owner) {

 69    function swapValidator(uint256 from_idx, uint256 to_idx) public onlyByOwnGov {

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

 95        bytes memory removed_pubkey = validators[remove_idx].pubKey;

108            Validator[] memory original_validators = validators;

181    function setWithdrawalCredential(bytes memory _new_withdrawal_pubkey) external onlyByOwnGov {

202    function setTimelock(address _timelock_address) external onlyByOwnGov {

208    event TimelockChanged(address timelock_address);

212    event ValidatorRemoved(bytes pubKey, uint256 remove_idx, bool dont_care_about_ordering);

214    event ValidatorsSwapped(bytes from_pubKey, bytes to_pubKey, uint256 from_idx, uint256 to_idx);

[N-06] Wrong event parameter name

Replace to parameter of TokenMinterBurned event to minter Replace from parameter of TokenMinterMinted event to minter

File: /src/ERC20/ERC20PermitPermissionedMint.sol

102    event TokenMinterBurned(address indexed from, address indexed to, uint256 amount);
103    event TokenMinterMinted(address indexed from, address indexed to, uint256 amount);

[N-07] Simplify depositWithSignature function

The parameter approveMax of depositWithSignature function could be remove, the permit assets should be always equal to deposit assets

File: /src/sfrxETH.sol

    /// @notice Approve and deposit() in one transaction
    function depositWithSignature(
        uint256 assets,
        address receiver,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external nonReentrant returns (uint256 shares) {
        asset.permit(msg.sender, address(this), assets, deadline, v, r, s);
        return (deposit(assets, receiver));
    }

Gas report

Total: 36 instances over 12 issues

[G-01] No need to initialize variables with default values

In solidity all variables initialize in 0, address(0), false, etc.

File: /src/frxETHMinter.sol

 63        withholdRatio = 0; // No ETH is withheld initially

 64        currentWithheldETH = 0;

 94        uint256 withheld_amt = 0;

129        for (uint256 i = 0; i < numDeposits; ++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/ERC20/ERC20PermitPermissionedMint.sol

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

[G‑02] ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

File: /src/ERC20/ERC20PermitPermissionedMint.sol

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

[G‑03] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop

File: /src/frxETHMinter.sol

129        for (uint256 i = 0; i < numDeposits; ++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/ERC20/ERC20PermitPermissionedMint.sol

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

[G‑04] <array>.length should not be looked up in every loop of a for-loop

The overheads outlined below are PER LOOP, excluding the first loop

• storage arrays incur a Gwarmaccess (100 gas)
• memory arrays use MLOAD (3 gas)
• calldata arrays use CALLDATALOAD (3 gas)

Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset

File: /src/OperatorRegistry.sol

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

File: /src/ERC20/ERC20PermitPermissionedMint.sol

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

[G‑05] Don't compare boolean expressions to boolean literals

require(<x> == true) => require(<x>) require(<x> == false) => require(!<x>)

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‑06] Unused minters_array variable

The data storage in minters_array it's never used also the minters mapping storage the same data

Remove the minters_array and his logic and use MinterAdded event to get off-chain the minters array

File: /src/ERC20/ERC20PermitPermissionedMint.sol

19    address[] public minters_array; // Allowed to mint

70        minters_array.push(minter_address);

83        // 'Delete' from the array by setting the address to 0x0
84        for (uint i = 0; i < minters_array.length; i++){
85            if (minters_array[i] == minter_address) {
86                minters_array[i] = address(0); // This will leave a null in the array and keep the indices the same
87                break;
88            }
89        }

[G-07] Overemited event parameter

In the Transfer event of openzeppelin-contracts/contracts/token/ERC20/ERC20.sol contract, the from, to and amount parameters are emited No need to emit them in the TokenMinterBurned and TokenMinterMinted events

Combime this with N-06

File: /src/ERC20/ERC20PermitPermissionedMint.sol

From:
102    event TokenMinterBurned(address indexed from, address indexed to, uint256 amount);
To:
102    event TokenMinterBurned(address indexed to);

From:
103    event TokenMinterMinted(address indexed from, address indexed to, uint256 amount);
To:
103    event TokenMinterMinted(address indexed from);

[G-08] Redundant require

The address(0) can be able to mint for the require in line 66

File: /src/ERC20/ERC20PermitPermissionedMint.sol

77        require(minter_address != address(0), "Zero address detected");

[G-09] Redundant onlyByOwnGov

This alaready cheks in addValidator functione in line 53

File: File: /src/OperatorRegistry.sol

61    function addValidators(Validator[] calldata validatorArray) external onlyByOwnGov {

[G-10] Using calldata instead of memory for read-only arguments in external functions saves gas

When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.

If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one

File: /src/OperatorRegistry.sol

172        bytes memory pubKey,

173        bytes memory signature,

181    function setWithdrawalCredential(bytes memory _new_withdrawal_pubkey) external onlyByOwnGov {

[G-11] public functions to external functions

File: /src/ERC20/ERC20PermitPermissionedMint.sol

53    function minter_burn_from(address b_address, uint256 b_amount) public onlyMinters {

59    function minter_mint(address m_address, uint256 m_amount) public onlyMinters {

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/OperatorRegistry.sol

82    function popValidators(uint256 times) public onlyByOwnGov {

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

File: /src/sfrxETH.sol

54    function pricePerShare() public view returns (uint256) {

[G-12] Use directly validators.length

This enables mark as external the numValidators function

File: /src/OperatorRegistry.sol

136        uint numVals = numValidators();

182        require(numValidators() == 0, "Clear validator array first");