Frax Ether Liquid Staking contest - __141345__'s results

Lines of code

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L78-L97

Vulnerability details

Impact

In the current rewards accounting, vault shares in deposit() and redeem() can not correctly record the spot yields generated by the staked asset. Yields are released over the next rewards cycle. As a result, malicious users can steal yields from innocent users by picking special timing to deposit() and redeem().

Proof of Concept

In syncRewards(), the current asset balance is break into 2 parts: storedTotalAssets and lastRewardAmount/nextRewards. The lastRewardAmount is the surplus balance of the asset, or the most recent yields.

// lib/ERC4626/src/xERC4626.sol
    function syncRewards() public virtual {
        // ...

        uint256 nextRewards = asset.balanceOf(address(this)) - storedTotalAssets_ - lastRewardAmount_;

        storedTotalAssets = storedTotalAssets_ + lastRewardAmount_;

        uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;

        lastRewardAmount = nextRewards.safeCastTo192();
        // ...        
        rewardsCycleEnd = end;
    }

And in the next rewards cycle, lastRewardAmount will be linearly added to storedTotalAssets, their sum is the return value of totalAssets():

    function totalAssets() public view override returns (uint256) {
        // ...

        if (block.timestamp >= rewardsCycleEnd_) {
            // no rewards or rewards fully unlocked
            // entire reward amount is available
            return storedTotalAssets_ + lastRewardAmount_;
        }

        // rewards not fully unlocked
        // add unlocked rewards to stored total
        uint256 unlockedRewards = (lastRewardAmount_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
        return storedTotalAssets_ + unlockedRewards;
    }

totalAssets() will be referred when deposit() and redeem().

// lib/solmate/src/mixins/ERC4626.sol

    function deposit(uint256 assets, address receiver) public virtual returns (uint256 shares) {
        require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
        // ...
        _mint(receiver, shares);
        // ...
    }

    function redeem() public virtual returns (uint256 assets) {
        // ...
        require((assets = previewRedeem(shares)) != 0, "ZERO_ASSETS");

        beforeWithdraw(assets, shares);

        _burn(owner, shares);

        // ...

        asset.safeTransfer(receiver, assets);
    }

    function previewDeposit(uint256 assets) public view virtual returns (uint256) {
        return convertToShares(assets);
    }

    function previewRedeem(uint256 shares) public view virtual returns (uint256) {
        return convertToAssets(shares);
    }

    function convertToShares(uint256 assets) public view virtual returns (uint256) {
        uint256 supply = totalSupply; 

        return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());
    }

    function convertToAssets(uint256 shares) public view virtual returns (uint256) {
        uint256 supply = totalSupply; 

        return supply == 0 ? shares : shares.mulDivDown(totalAssets(), supply);
    }

Based on the above rules, there are 2 potential abuse cases:

1. If withdraw just after the rewardsCycleEnd timestamp, a user can not get the yields from last rewards cycle. Since the totalAssets() only contain storedTotalAssets but not the yields part. It takes 1 rewards cycle to linearly add to the storedTotalAssets.

Assume per 10,000 asset staking generate yields of 70 for 7 days, and the reward cycle is 1 day. A malicious user Alice can do the following:

• watch the mempool for withdraw(10,000) from account Bob, front run it with syncRewards(), so that the most recent yields of amount 70 from Bob will stay in the vault.
• Alice will also deposit a 10,000 to take as much shares as possible.
• after 1 rewards cycle of 1 day, redeem() to take the yields of 70.

Effectively steal the yields from Bob. The profit for Alice is not 70, because after 1 day, her own deposit also generates some yield, in this example this portion is 1. At the end, Alice steal yield of amount 60.

2. When the Multisig Treasury transfers new yields into the vault, the new yields will accumulate until syncRewards() is called. It is possible that yields from multiple rewards cycles accumulates, and being released in the next cycle.

Knowing that the yields has been accumulated for 3 rewards cycles, a malicious user can deposit() and call syncRewards() to trigger the release of the rewards. redeem() after 1 cycle.

Here the malicious user gets yields of 3 cycles, lose 1 in the waiting cycle. The net profit is 2 cycle yields, and the gained yields should belong to the other users in the vault.

Tools Used

Manual analysis.

Recommended Mitigation Steps

• for the lastRewardAmount not released, allow the users to redeem as it is linearly released later.
• for the accumulated yields, only allow users to redeem the yields received after 1 rewards cycle after the deposit.

Lines of code

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

Vulnerability details

Impact

There is no guarantee that the validators will not lose fund. There are multiple pitfalls can result in loss, according to Ethereum document: penalties could come from slashing, inactivity leak, reference. Or even unexpected bugs can cause loss to the validators, such as this news: Seventy-five Eth2 validators got slashed this week due to a bug witnessed by Staked.

If some validators did incur losses, or lots of the validators suffer loss due to some softwares bugs, the equity of the frxETH holders could be less than what they have deposited. Then the booking value of the frxETH can not support the 1:1 peg, off-peg could be triggered, the consequence might go out of control. Because at the same time, the multisig is still sending staking rewards ETH to the frxETH contract to mint with hardcoded 1:1 exchange ratio. For the sfrxETH holders, what they receive is much over valued. To stop loss, they will tend to exit as soon as possible. If this scenario happens, it is expected to have large scale withdraw() of the vault, followed by panic sell off, and eventually even further depreciation of frxETH and market collapse.

If the vault was withdraw() to empty, the potential issue of ERC4626 vault exchange rate manipulation could also happen, further harm the vault and other users.

This issue is deemed as High because once it happens, the impacts might be catastrophic to the protocol.

Proof of Concept

The exchange rate of frxETH to ETH is hardcoded. During off-peg period, the incorrect ratio could enhance the market tumble.

// src/frxETHMinter.sol
    function submitAndDeposit(address recipient) external payable returns (uint256 shares) {
        // ...
        uint256 sfrxeth_recieved = sfrxETHToken.deposit(msg.value, recipient);
        // ...
    }

Tools Used

Manual analysis.

Recommended Mitigation Steps

• monitor the under-performed validators and stop loss timely.
• maybe change the hardcoded frxETH to ETH exchange ratio, and allow for some variation. Then market trades might help to stabilize the price.
• buy insurance as downside protection for off-peg, for potential large scale validator loss. Otherwise, no base booking value could result in disastrous results like UST/luna.

Lines of code

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

Vulnerability details

Impact

When depositEther(), if 1 validators is used before, the whole deposit function will revert, causing DoS. depositEther() function will be inoperable until the gov manually remove the mistaken validator.

Proof of Concept

In depositEther(), if the pubKey is already used, the whole loop will revert, and the deposit operation cannot move on.

// src/frxETHMinter.sol
    function depositEther() external nonReentrant {
        // ...

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

And in the next rewards cycle, lastRewardAmount will be linearly added to storedTotalAssets, their sum is the return value of totalAssets():

    function totalAssets() public view override returns (uint256) {
        // ...

        if (block.timestamp >= rewardsCycleEnd_) {
            // no rewards or rewards fully unlocked
            // entire reward amount is available
            return storedTotalAssets_ + lastRewardAmount_;
        }

        // rewards not fully unlocked
        // add unlocked rewards to stored total
        uint256 unlockedRewards = (lastRewardAmount_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
        return storedTotalAssets_ + unlockedRewards;
    }

Temporarily the depositEther() function will be inaccessible. Until the governance calls the registry to pop the wrong validator.

// src/OperatorRegistry.sol
    function popValidators(uint256 times) public onlyByOwnGov {
        // Loop through and remove validator entries at the end
        for (uint256 i = 0; i < times; ++i) {
            validators.pop();
        }

        emit ValidatorsPopped(times);
    }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Use try/catch to skip the wrong validator, then the deposit function will be more robust to unexpected situations.

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.

Instances number of this issue:

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

// src/frxETHMinter.sol
207:    event ETHSubmitted(address indexed sender, address indexed recipient, uint256 sent_amount, uint256 withheld_amt);

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

Code Structure Deviates From Best-Practice

The best-practice layout for a contract should follow the following order: state variables, events, modifiers, constructor and functions. Function ordering helps readers identify which functions they can call and find constructor and fallback functions easier. Functions should be grouped according to their visibility and ordered as: constructor, receive function (if exists), fallback function (if exists), external, public, internal, private. Some constructs deviate from this recommended best-practice: Modifiers are in the middle of contracts. External/public functions are mixed with internal/private ones. Few events are declared in contracts while most others are in corresponding interfaces.

Suggestion: Consider adopting recommended best-practice for code structure and layout.

Use a more recent version of solidity

Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings.

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value.

Description: Using very old versions of Solidity prevents benefits of bug fixes and newer security checks. Using the latest versions might make contracts susceptible to undiscovered compiler bugs.

Suggestion: Consider using the most recent version.

Avoid floating pragmas: the version should be locked

The pragma declared across the contracts is ^0.8.0. Locking the pragma (for e.g. by not using ^ in pragma solidity 0.8.0) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs. (see here)

Description: 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.

Suggestion: Consider locking the pragma version.

Missing Time locks

When critical parameters of systems need to be changed, it is required to broadcast the change via event emission and recommended to enforce the changes after a time-delay. This is to allow system users to be aware of such critical changes and give them an opportunity to exit or adjust their engagement with the system accordingly. None of the onlyOwner functions that change critical protocol addresses/parameters have a timelock for a time-delayed change to alert: (1) users and give them a chance to engage/exit protocol if they are not agreeable to the changes (2) team in case of compromised owner(s) and give them a chance to perform incident response.

Instances number of this issue:

// src/frxETHMinter.sol
    function setWithholdRatio(uint256 newRatio) external onlyByOwnGov {}

    function togglePauseSubmits() external onlyByOwnGov {}

    function togglePauseDepositEther() external onlyByOwnGov {}

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

Suggestion: Users may be surprised when critical parameters are changed. Furthermore, it can erode users' trust since they can’t be sure the protocol rules won’t be changed later on. Compromised owner keys may be used to change protocol addresses/parameters to benefit attackers. Without a time-delay, authorized owners have no time for any planned incident response. (May be medium since owner can change critical parameters at anytime that can affect the users poorly).

for-loop array.length should not be looked up in every loop

Even for memory variables. The demo of the loop gas comparison can be seen here.

Instances number of this issue: 2

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

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

No need to explicitly initialize variables with default values

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

Instances number of this issue: 4

// src/ERC20/ERC20PermitPermissionedMint.sol
84:     for (uint i = 0; i < minters_array.length; 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) {

The demo of the loop gas comparison can be seen here.

++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

This saves 30-40 gas per loop

Instances number of this issue: 4

// src/ERC20/ERC20PermitPermissionedMint.sol
84:     for (uint i = 0; i < minters_array.length; 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) {

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

Saves 5 gas per loop

Instances number of this issue: 4

// src/ERC20/ERC20PermitPermissionedMint.sol
84:     for (uint i = 0; i < minters_array.length; 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) {

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

If a function modifier such as onlyMinters() 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 legitimate callers because the compiler will not include checks for whether a payment was provided.

The extra opcodes avoided are

CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2)

which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

Instances number of this issue: 18

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

// src/frxETHMinter.sol
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 {

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

Using bool for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full

// word because each write operation emits an extra SLOAD to first read the
// slot's contents, replace the bits taken up by the boolean, and then write
// back. This is the compiler's defense against contract upgrades and
// pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for 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

Instances number of this issue: 2

// src/frxETHMinter.sol
49:     bool public submitPaused;
50:     bool public depositEtherPaused;

Use custom errors rather than revert()/require() strings

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas

Instances number of this issue: 15

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

require()/revert() strings longer than 32 bytes cost extra gas

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas.

Instances number of this issue: 1

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

Using calldata instead of memory for read-only arguments in external functions

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 contracts 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 gas-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

Note that here also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved

Instances number of this issue: 3

// src/OperatorRegistry.sol
151-159:
        function getValidator(uint i) 
            view
            external
            returns (
                bytes memory pubKey,
                bytes memory withdrawalCredentials,
                bytes memory signature,
                bytes32 depositDataRoot
            )
171-175:
        function getValidatorStruct(
            bytes memory pubKey, 
            bytes memory signature, 
            bytes32 depositDataRoot
        ) external pure returns (Validator memory) {        
181:    function setWithdrawalCredential(bytes memory _new_withdrawal_pubkey) external onlyByOwnGov {

Use private rather than public for constants

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

Instances number of this issue: 1

// src/frxETHMinter.sol
39:   uint256 public constant RATIO_PRECISION = 1e6; // 1,000,000 

Duplicate calculation

storedTotalAssets_ + lastRewardAmount_ is calculated twice, the line storedTotalAssets = storedTotalAssets_ + lastRewardAmount_; can be moved before nextRewards.

// lib/ERC4626/src/xERC4626.sol
    function syncRewards() public virtual {
        // ...
85:     uint256 nextRewards = asset.balanceOf(address(this)) - storedTotalAssets_ - lastRewardAmount_;

87:     storedTotalAssets = storedTotalAssets_ + lastRewardAmount_; // SSTORE

        // ...
    }