Frax Ether Liquid Staking contest - rbserver's results

A liquid ETH staking derivative designed to uniquely leverage the Frax Finance ecosystem.

General Information

Platform: Code4rena

Start Date: 22/09/2022

Pot Size: $30,000 USDC

Total HM: 12

Participants: 133

Period: 3 days

Judge: 0xean

Total Solo HM: 2

Id: 165

League: ETH

Frax Finance

Findings Distribution

Researcher Performance

Rank: 30/133

Findings: 4

Award: $100.33

🌟 Selected for report: 0

🚀 Solo Findings: 0

Awards

19.982 USDC - $19.98

Labels

bug
duplicate
2 (Med Risk)

External Links

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 https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L199-L203 https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol#L45-L48

Vulnerability details

Impact

The owner of the frxETHMinter contract is able to call the following moveWithheldETH, recoverEther, and recoverERC20 functions as allowed by the onlyByOwnGov modifier below. There is no guarantee that the owner will not become compromised or malicious in the future. After becoming compromised or malicious, the owner can call moveWithheldETH, recoverEther, or recoverERC20 to transfer funds from the frxETHMinter contract to an address of choice. As a result, the ETH submitted by the users are stolen and are not used for any staking. Similarly, the ERC20 tokens that are accidentally sent to the frxETHMinter contract can never be returned to the senders.

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

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

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

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

        emit EmergencyEtherRecovered(amount);
    }

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

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

    modifier onlyByOwnGov() {
        require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");
        _;
    }

Proof of Concept

Please append the following tests in the test\frxETHMinter.t.sol. These tests will pass to demonstrate the described scenarios for calling moveWithheldETH and recoverEther. The scenario for calling recoverERC20 is similar to these cases.

    function testCallmoveWithheldETHByCompromisedOrMaliciousOwner() public {
        // simulate users
        address user1 = address(1);
        address user2 = address(2);
        vm.deal(user1, 100 ether);
        vm.deal(user2, 100 ether);

        // set withhold ratio to 50%
        vm.prank(FRAX_COMPTROLLER);
        minter.setWithholdRatio(500_000);

        // users call submit
        vm.prank(user1);
        minter.submit{ value: 32 ether }();

        vm.prank(user2);
        minter.submit{ value: 32 ether }();

        // minter owns 64 ether at this moment
        assertEq(address(minter).balance, 64 ether);

        // of the 64 ether, 32 ether are withheld at this moment
        assertEq(minter.currentWithheldETH(), 32 ether);

        // simulate receiving address for the exploit
        address payable exploitReceiver = payable(address(123));

        // simulate a situation in which the owner is compromised or becomes malicious
        // in this situation, this owner calls moveWithheldETH, which takes effect immediately
        vm.prank(FRAX_COMPTROLLER);
        minter.moveWithheldETH(exploitReceiver, 32 ether);

        // after the owner calls moveWithheldETH, exploitReceiver receives the 32 ether that was minter's currentWithheldETH
        assertEq(address(minter).balance, 32 ether);
        assertEq(minter.currentWithheldETH(), 0);
        assertEq(address(exploitReceiver).balance, 32 ether);
    }
    function testCallrecoverEtherByCompromisedOrMaliciousOwner() public {
        // simulate users
        address user1 = address(1);
        address user2 = address(2);
        vm.deal(user1, 100 ether);
        vm.deal(user2, 100 ether);

        // set withhold ratio to 50%
        vm.prank(FRAX_COMPTROLLER);
        minter.setWithholdRatio(500_000);

        // users call submit
        vm.prank(user1);
        minter.submit{ value: 32 ether }();

        vm.prank(user2);
        minter.submit{ value: 32 ether }();

        // minter owns 64 ether at this moment
        assertEq(address(minter).balance, 64 ether);

        uint256 ownerBalanceBefore = address(FRAX_COMPTROLLER).balance;

        // simulate a situation in which the owner is compromised or becomes malicious
        // in this situation, this owner calls recoverEther, which takes effect immediately
        vm.prank(FRAX_COMPTROLLER);
        minter.recoverEther(64 ether);

        // after the owner calls recoverEther, the owner receives the 64 ether that was minter's ETH balance
        assertEq(address(minter).balance, 0);
        assertEq(address(FRAX_COMPTROLLER).balance, ownerBalanceBefore + 64 ether);
    }

Tools Used

VSCode

The moveWithheldETH, recoverEther, and recoverERC20 functions can be updated to be only callable by the governance timelock contract, instead of beling also callable by the owner. This ensures that calling these functions are authorized by the governance.

#0 - FortisFortuna

2022-09-25T21:20:10Z

We are well aware of the permission structure. The owner will most likely be a large multisig. We mentioned the Frax Multisig in the scope too.

#1 - joestakey

2022-09-26T16:05:53Z

Duplicate of #107

Findings Information

🌟 Selected for report: Lambda

Also found by: 0x52, Bahurum, Bnke0x0, KIntern_NA, Respx, Soosh, TomJ, Trust, V_B, lukris02, rbserver, rotcivegaf, yixxas

Labels

bug
duplicate
2 (Med Risk)
sponsor confirmed
depositEther OOG

Awards

39.1574 USDC - $39.16

External Links

Lines of code

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

Vulnerability details

Impact

When calling the following depositEther function, each deposit chunk is iterated over. In each iteration, the getNextValidator function below is called to pop the last validator from the validators state variable, which is defined in the OperatorRegistry contract, and return it for the ETH 2.0 staking purpose. When the frxETHMinter contract holds enough ETH, it is possible that the number of deposit chunks is more than the number of the added validators. In this case, calling depositEther would revert even though that there are enough validators for staking some of the deposit chunks. As a result, none of the ETH that are submitted by the users can earn yield from ETH 2.0 staking at that moment. Even if more validators can be added later to match the number of deposit chunks, the deposit chunks that could be staked by using the current validators are not utilized at all, and the time spent for searching and adding more validators is wasted because it could be used for earning the yield already for such deposit chunks.

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

    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;

            emit DepositSent(pubKey, withdrawalCredential);
        }
    }

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

    function getNextValidator()
        internal
        returns (
            bytes memory pubKey,
            bytes memory withdrawalCredentials,
            bytes memory signature,
            bytes32 depositDataRoot
        )
    {
        // Make sure there are free validators available
        uint numVals = numValidators();
        require(numVals != 0, "Validator stack is empty");

        // Pop the last validator off the array
        Validator memory popped = validators[numVals - 1];
        validators.pop();

        // Return the validator's information
        pubKey = popped.pubKey;
        withdrawalCredentials = curr_withdrawal_pubkey;
        signature = popped.signature;
        depositDataRoot = popped.depositDataRoot;
    }

Proof of Concept

Please append the following test in the test\frxETHMinter.t.sol. This test will pass to demonstrate the described scenario.

    function testdepositEtherTransactionRevertsWhenValidatorCountIsLessThanDepositChunkCount() public {
        // simulate users
        address user1 = address(1);
        address user2 = address(2);
        address user3 = address(3);
        vm.deal(user1, 100 ether);
        vm.deal(user2, 100 ether);
        vm.deal(user3, 100 ether);

        vm.startPrank(FRAX_COMPTROLLER);
        vm.deal(FRAX_COMPTROLLER, 320 ether);
        
        // add validators
        minter.addValidator(OperatorRegistry.Validator(pubKeys[0], sigs[0], ddRoots[0]));
        minter.addValidator(OperatorRegistry.Validator(pubKeys[1], sigs[1], ddRoots[1]));

        // set withhold ratio to 50%
        minter.setWithholdRatio(500_000);
        vm.stopPrank();

        // users call submit
        vm.prank(user1);
        minter.submit{ value: 64 ether }();

        vm.prank(user2);
        minter.submit{ value: 64 ether }();

        vm.prank(user3);
        minter.submit{ value: 64 ether }();

        // minter owns 192 ether at this moment
        assertEq(address(minter).balance, 192 ether);

        // of the 192 ether, 96 ether are withheld at this moment
        assertEq(minter.currentWithheldETH(), 96 ether);

        // calling depositEther reverts because the number of validators is less than the number of deposit chunks
        vm.expectRevert("Validator stack is empty");
        vm.prank(user2);
        minter.depositEther();

        // minter still owns 192 ether after the reversion
        assertEq(address(minter).balance, 192 ether);

        // of the 192 ether, 96 ether are still withheld after the reversion
        assertEq(minter.currentWithheldETH(), 96 ether);
    }

Tools Used

VSCode

validators can be updated to be internal in the OperatorRegistry contract. Then, when calling the depositEther function in the frxETHMinter contract, for (uint256 i = 0; i < validators.length; ++i) { ... } can be executed instead of for (uint256 i = 0; i < numDeposits; ++i) { ... }.

#0 - FortisFortuna

2022-09-25T22:45:34Z

We plan to keep an eye on the number free validators and have a decent sized buffer of them.

#1 - FortisFortuna

2022-09-26T16:31:06Z

Adding a maxLoops parameter or similar can help mitigate this for sure.

#2 - FortisFortuna

2022-09-26T17:22:30Z

#3 - 0xean

2022-10-11T21:37:47Z

ref #224

[L-01] CALLING togglePauseSubmits OR togglePauseDepositEther FUNCTION BY OWNER TAKES EFFECT IMMEDIATELY, WHICH LEAVES NO TIME FOR USERS TO REACT

The following togglePauseSubmits and togglePauseDepositEther functions are callable by the owner as allowed by the onlyByOwnGov modifier that is used by these functions. Unlike being called by the governance timelock contract, calling one of these functions by the owner will take effect immediately. Before calling the _submit or depositEther function below, the user can check the submitPaused or depositEtherPaused value at that moment; if such value is false, then she or he will send the _submit or depositEther transaction. However, without any prior notice, the owner calls togglePauseSubmits or togglePauseDepositEther just before the user calls _submit or depositEther. As a result, the user's transaction reverts when calling _submit or depositEther. The user wastes gas and feels that the user experience is degraded. To mitigate the centralization risk and build trust among users, please consider making the togglePauseSubmits and togglePauseDepositEther functions only callable by the governance timelock contract.

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

    function togglePauseSubmits() external onlyByOwnGov {
        submitPaused = !submitPaused;

        emit SubmitPaused(submitPaused);
    }

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

    function togglePauseDepositEther() external onlyByOwnGov {
        depositEtherPaused = !depositEtherPaused;

        emit DepositEtherPaused(depositEtherPaused);
    }

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

    function _submit(address recipient) internal nonReentrant {
        // Initial pause and value checks
        require(!submitPaused, "Submit is paused");
        ...
    }

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

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

        ...
    }

[L-02] CALLING moveWithheldETH FUNCTION CAN REVERT IF THE to INPUT CORRESPONDS TO A CONTRACT THAT DOES NOT INTEND TO RECEIVE ETH THROUGH ITS receive OR fallback FUNCTION

Calling the following moveWithheldETH function will send amount ETH from the frxETHMinter contract to the to address. It is possible that to corresponds to a contract that does not intend to receive ETH through its receive or fallback function. In this case, calling moveWithheldETH will revert. To allow sending ETH to such contract, please consider adding another function, which is similar to moveWithheldETH, that has a data input in which (bool success,) = payable(to).call{ value: amount }(data) can be executed for sending ETH through calling such contract's relevant function.

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

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

[L-03] MISSING ZERO-ADDRESS CHECKS FOR CRITICAL ADDRESSES

To prevent unintended behaviors, critical address inputs should be checked against address(0).

Please consider checking _timelock_address in the following constructor. https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETH.sol#L36-L41

    constructor(
      address _creator_address,
      address _timelock_address
    ) 
    ERC20PermitPermissionedMint(_creator_address, _timelock_address, "Frax Ether", "frxETH") 
    {}

Please consider checking depositContractAddress, frxETHAddress, sfrxETHAddress, and _timelock_address in the following constructor. https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L52-L65

    constructor(
        address depositContractAddress, 
        address frxETHAddress, 
        address sfrxETHAddress, 
        address _owner, 
        address _timelock_address,
        bytes memory _withdrawalCredential
    ) OperatorRegistry(_owner, _timelock_address, _withdrawalCredential) {
        depositContract = IDepositContract(depositContractAddress);
        frxETHToken = frxETH(frxETHAddress);
        sfrxETHToken = IsfrxETH(sfrxETHAddress);
        withholdRatio = 0; // No ETH is withheld initially
        currentWithheldETH = 0;
    }

[N-01] CONSTANTS CAN BE USED INSTEAD OF MAGIC NUMBERS

To improve readability and maintainability, constants can be used instead of magic numbers. Please consider replacing the magic numbers used in the following code with constants.

https://github.com/code-423n4/2022-09-frax/blob/main/src/sfrxETH.sol#L54-L56

    function pricePerShare() public view returns (uint256) {
        return convertToAssets(1e18);
    }

https://github.com/corddry/ERC4626/blob/main/src/xERC4626.sol#L91-L93

        if (end - timestamp < rewardsCycleLength / 20) {
            end += rewardsCycleLength;
        }

[N-02] MISSING INDEXED EVENT FIELDS

Querying events can be optimized with index. Please consider adding indexed to the relevant fields of the following events.

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

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

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

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

[N-03] MISSING NATSPEC COMMENTS

NatSpec comments provide rich code documentation. NatSpec comments are missing for the following functions. Please consider adding them.

lib\ERC4626\src\xERC4626.sol
  65: function beforeWithdraw(uint256 amount, uint256 shares) internal virtual override {
  71: function afterDeposit(uint256 amount, uint256 shares) internal virtual override {

[N-04] INCOMPLETE NATSPEC COMMENTS

NatSpec comments provide rich code documentation. @param or @return comments are missing for the following functions. Please consider completing NatSpec comments for them.

src\frxETHMinter.sol
  70: function submitAndDeposit(address recipient) external payable returns (uint256 shares) {    
  85: function _submit(address recipient) internal nonReentrant {    
  109: function submitAndGive(address recipient) external payable {    
  166: function moveWithheldETH(address payable to, uint256 amount) external onlyByOwnGov {    
  191: function recoverEther(uint256 amount) external onlyByOwnGov {   

src\OperatorRegistry.sol
  53: function addValidator(Validator calldata validator) public onlyByOwnGov {   
  69: function swapValidator(uint256 from_idx, uint256 to_idx) public onlyByOwnGov {  
  93: function removeValidator(uint256 remove_idx, bool dont_care_about_ordering) public onlyByOwnGov {  
  126: function getNextValidator() 
  151: function getValidator(uint i)  
  171: function getValidatorStruct( 
  181: function setWithdrawalCredential(bytes memory _new_withdrawal_pubkey) external onlyByOwnGov { 
  197: function numValidators() public view returns (uint256) {    

src\sfrxETH.sol
  48: function beforeWithdraw(uint256 assets, uint256 shares) internal override {    
  54: function pricePerShare() public view returns (uint256) {    
  59: function depositWithSignature(    
  75: function mintWithSignature(    

lib\ERC4626\src\xERC4626.sol
  45: function totalAssets() public view override returns (uint256) {

[N-05] FLOATING PRAGMAS

It is a best practice to lock pragmas instead of using floating pragmas to ensure that contracts are tested and deployed with the intended compiler version. Accidentally deploying contracts with different compiler versions can lead to unexpected risks and undiscovered bugs. Please consider locking pragma for the following files.

src\frxETHMinter.sol
  2: pragma solidity ^0.8.0;

src\OperatorRegistry.sol
  2: pragma solidity ^0.8.0;

src\sfrxETH.sol
  2: pragma solidity ^0.8.0;

src\ERC20\ERC20PermitPermissionedMint.sol
  2: pragma solidity ^0.8.0;

lib\ERC4626\src\xERC4626.sol
  4: pragma solidity ^0.8.0;

[G-01] STATE VARIABLES THAT ARE ACCESSED FOR MULTIPLE TIMES CAN BE CACHED IN MEMORY

State variables that are accessed for multiple times can be cached in memory, and the cached variable value can then be used. This can replace multiple sload operations with one sload, one mstore, and multiple mload operations to save gas.

withholdRatio can be cached, and the cached value can then be used in the following _submit function. https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L85-L101

    function _submit(address recipient) internal nonReentrant {
        ...

        // Track the amount of ETH that we are keeping
        uint256 withheld_amt = 0;
        if (withholdRatio != 0) {
            withheld_amt = (msg.value * withholdRatio) / RATIO_PRECISION;
            currentWithheldETH += withheld_amt;
        }

        emit ETHSubmitted(msg.sender, recipient, msg.value, withheld_amt);
    }

submitPaused can be cached, and the cached value can then be used in the following togglePauseSubmits function. https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L177-L181

    function togglePauseSubmits() external onlyByOwnGov {
        submitPaused = !submitPaused;

        emit SubmitPaused(submitPaused);
    }

depositEtherPaused can be cached, and the cached value can then be used in the following togglePauseDepositEther function. https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L184-L188

    function togglePauseDepositEther() external onlyByOwnGov {
        depositEtherPaused = !depositEtherPaused;

        emit DepositEtherPaused(depositEtherPaused);
    }

[G-02] ARRAY LENGTH CAN BE CACHED OUTSIDE OF LOOP

Caching the array length outside of the loop and using the cached length in the loop costs less gas than reading the array length for each iteration. For example, minters_array.length in the following code can be cached outside of the loop like uint256 mintersArrayLength = minters_array.length, and i < mintersArrayLength can be used for each iteration.

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

[G-03] ARITHMETIC OPERATIONS THAT DO NOT OVERFLOW CAN BE UNCHECKED

Explicitly unchecking arithmetic operations that do not overflow by wrapping these in unchecked {} costs less gas than implicitly checking these.

For the following loops, unchecked {++i} at the end of the loop block can be used.

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

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

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

[G-04] VARIABLE DOES NOT NEED TO BE INITIALIZED TO ITS DEFAULT VALUE

Explicitly initializing a variable with its default value costs more gas than uninitializing it. For example, uint256 i can be used instead of uint256 i = 0 in the following code.

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

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

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

[G-05] ++VARIABLE CAN BE USED INSTEAD OF VARIABLE++

++variable costs less gas than variable++. For example, i++ can be changed to ++i in the following code.

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

[G-06] X = X + Y OR X = X - Y CAN BE USED INSTEAD OF X += Y OR X -= Y

x = x + y or x = x - y costs less gas than x += y or x -= y. For example, currentWithheldETH += withheld_amt can be changed to currentWithheldETH = currentWithheldETH + withheld_amt in the following code.

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

[G-07] REVERT REASON STRINGS CAN BE SHORTENED TO FIT IN 32 BYTES

Revert reason strings that are longer than 32 bytes need more memory space and more mstore operation than these that are shorter than 32 bytes when reverting. Please consider shortening the following revert reason string.

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

        require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");

[G-08] REVERT WITH CUSTOM ERROR CAN BE USED INSTEAD OF REQUIRE() WITH REASON STRING

revert with custom error can cost less gas than require() with reason string. Please consider using revert with custom error to replace the following require().

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

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

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

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter