xETH - Versus Contest - adriro's results

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L239 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L286

Vulnerability details

Rebalance amounts should be checked so that updated balances falls within thresholds

Rebalance operations are allowed when the current percentage of xETH in the Curve pool is outside the defined thresholds. However, there is no check to ensure that the amount of xETH added or burned from the pool will update the percentage to be within the limits.

Impact

Rebalance actions in the AMO contract are first validated to ensure that a rebalance is indeed needed in the first place. This is implemented by calculating the percentage of xETH in the pool, and comparing the result against two limits that define an acceptable range.

Rebalance up is allowed when the percentage of xETH in the Curve pool is above the REBALANCE_UP_THRESHOLD, in which case xETH is withdrawn from the pool and burned to lower the percentage. Similarly, rebalance down is allowed when the percentage falls below the REBALANCE_DOWN_THRESHOLD value, in which case xETH is minted and added to the pool, increasing the percentage.

While care is taken to ensure that these operations are indeed only allowed when the percentages are outside the defined bounds, there is no check to ensure that the amount of xETH burned or minted will update the percentage to be within the bounds.

Proof of concept

During a rebalance operation, a malicious or improperly operating defender can exploit the opportunity to modify the pool balance in their favor, potentially exceeding the desired boundaries.

For example, if xEthPct > REBALANCE_UP_THRESHOLD is true, the preRebalanceCheck() function would allow a rebalanceUp() operation. In such a scenario, the defender could deliberately lower the percentage to below the desired REBALANCE_DOWN_THRESHOLD, potentially gaining an advantage if the account is compromised or if there is a malfunction in the bot operating off-chain.

Recommendation

Add a check to ensure that updated balances comply with the thresholds boundaries. When minting or burning xETH from the pool, the updated balance of xETH should produce a percentage that is within the accepted range.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L256-L259 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L600-L604 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L632-L636

Vulnerability details

Inconsistent check for LP balance in AMO

While pulling LP tokens from the CVXStaker contract, the AMO queries the current available balance using the staked balance, which is inconsistent with the implementation of the withdraw function.

Impact

Curve LP tokens owned by the AMO contract are staked in a Convex pool that is handled using the CVXStaker contract. When liquidity needs to be removed from the Curve pool, the AMO contract needs to first withdraw the LP tokens from the CVXStaker contract.

The rebalanceUp(), removeLiquidity() and removeLiquidityOnlyStETH() functions present in the AMO contract deal with removing liquidity from the Curve pool. In their implementations, all of them query the available LP balance using the stakedBalance() function of CVXStaker. Taking the rebalanceUp() function as an example (other cases are similar), we can see the following:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L256-L261

...
256:         uint256 amoLpBal = cvxStaker.stakedBalance();
257: 
258:         // if (amoLpBal == 0 || quote.lpBurn > amoLpBal) revert LpBalanceTooLow();
259:         if (quote.lpBurn > amoLpBal) revert LpBalanceTooLow();
260: 
261:         cvxStaker.withdrawAndUnwrap(quote.lpBurn, false, address(this));
...

The implementation of stakedBalance() basically delegates the call to fetch the staked balance in the Convex reward pool contract:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L204-L206

204:     function stakedBalance() public view returns (uint256 balance) {
205:         balance = IBaseRewardPool(cvxPoolInfo.rewards).balanceOf(address(this));
206:     }

However, this check is not correct. As we can see in the implementation of withdrawAndUnwrap(), the CVXStaker contract consider not only staked tokens, but also available balance held in the contract itself:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L142-L161

142:     function withdrawAndUnwrap(
143:         uint256 amount,
144:         bool claim,
145:         address to
146:     ) external onlyOperatorOrOwner {
147:         // Optimistically use CLP balance in this contract, and then try and unstake any remaining
148:         uint256 clpBalance = clpToken.balanceOf(address(this));
149:         uint256 toUnstake = (amount < clpBalance) ? 0 : amount - clpBalance;
150:         if (toUnstake > 0) {
151:             IBaseRewardPool(cvxPoolInfo.rewards).withdrawAndUnwrap(
152:                 toUnstake,
153:                 claim
154:             );
155:         }
156: 
157:         if (to != address(0)) {
158:             // unwrapped amount is 1 to 1
159:             clpToken.safeTransfer(to, amount);
160:         }
161:     }

Line 148 considers potentially available LP tokens in the contract, and withdraws the remaining amount from Convex.

This means that checking against stakedBalance() is too restrictive and incorrect, and can potentially lead to situations in which the required LP tokens are enough, but the check in line 259 of the AMO contract will revert the operation.

Proof of concept

As an example, let's take a call to the rebalanceUp() function and assume that the quoted lpBurn amount is 4. The CVXStaker contract has 3 LP tokens staked in Convex and 2 tokens held as balance in the contract.

In this situation, the condition quote.lpBurn > amoLpBal will be true, as amoLpBal = cvxStaker.stakedBalance() = 3, which evaluates the condition to 5 > 3, causing a revert in the transaction.

However, the operation would succeed if the check weren't there, as withdrawAndUnwrap() will first consider the 2 tokens already present in the CVXStaker contract and withdraw the remaining 2 from the Convex pool, successfully fulfilling the requested amount.

Recommendation

The validation to ensure available LP tokens in rebalanceUp(), removeLiquidity() and removeLiquidityOnlyStETH() should not only consider stakedBalance() but also available LP tokens present in the CVXStaker contract. Alternatively, the check can be removed as the call to withdrawAndUnwrap() will eventually fail if the available tokens are not enough.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L185-L198

Vulnerability details

Zero token transfer can cause a potential DoS in CVXStaker

The CVXStaker contract doesn't check for zero amount while transferring rewards, which can end up blocking the operation.

Impact

The CVXStaker contract is in charge of handling interaction with the Convex pool. The getReward() function is used to claim rewards and transfer them to the rewards recipient:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L185-L198

185:     function getReward(bool claimExtras) external {
186:         IBaseRewardPool(cvxPoolInfo.rewards).getReward(
187:             address(this),
188:             claimExtras
189:         );
190:         if (rewardsRecipient != address(0)) {
191:             for (uint i = 0; i < rewardTokens.length; i++) {
192:                 uint256 balance = IERC20(rewardTokens[i]).balanceOf(
193:                     address(this)
194:                 );
195:                 IERC20(rewardTokens[i]).safeTransfer(rewardsRecipient, balance);
196:             }
197:         }
198:     }

As we can see in the previous snippet of code, the implementation will loop all the configured reward tokens and transfer them one by one to the reward recipient.

This is a bit concerning as some ERC20 implementations revert on zero value transfers (see https://github.com/d-xo/weird-erc20#revert-on-zero-value-transfers). If at least one of the reward tokens includes this behavior, then the current implementation may cause a denial of service, as a zero amount transfer in this token will block the whole action and revert the transaction.

Note that the rewards array is not modifiable, it is defined at construction time, a token cannot be removed.

Proof of concept

We reproduce the issue in the following test. token1 is a normal ERC20 and token2 reverts on zero transfer amounts. Rewards from token1 can't be transferred to the recipient as the zero transfer on token2 will revert the operation.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_CVXStaker_RevertOnZeroTokenTransfer() public {
    MockErc20 token1 = new MockErc20("Token1", "TOK1", 18);
    MockErc20 token2 = new RevertOnZeroErc20("Token2", "TOK2", 18);

    MockCVXRewards rewards = new MockCVXRewards();

    address operator = makeAddr("operator");
    IERC20 clpToken = IERC20(makeAddr("clpToken"));
    ICVXBooster booster = ICVXBooster(makeAddr("booster"));
    address[] memory rewardTokens = new address[](2);
    rewardTokens[0] = address(token1);
    rewardTokens[1] = address(token2);

    CVXStaker staker = new CVXStaker(operator, clpToken, booster, rewardTokens);
    staker.setCvxPoolInfo(0, address(clpToken), address(rewards));

    address rewardsRecipient = makeAddr("rewardsRecipient");
    staker.setRewardsRecipient(rewardsRecipient);

    // simulate staker has some token1 but zero token2 after calling getRewards
    deal(address(token1), address(staker), 1e18);

    // The transaction will fail as the implementation will try to transfer zero
    // tokens for token2, blocking the whole operation.
    vm.expectRevert("cannot transfer zero amount");
    staker.getReward(true);
}

Recommendation

Check for zero amount before executing the transfer:

function getReward(bool claimExtras) external {
    IBaseRewardPool(cvxPoolInfo.rewards).getReward(
        address(this),
        claimExtras
    );
    if (rewardsRecipient != address(0)) {
        for (uint i = 0; i < rewardTokens.length; i++) {
            uint256 balance = IERC20(rewardTokens[i]).balanceOf(
                address(this)
            );
            
+           if (balance > 0) {
              IERC20(rewardTokens[i]).safeTransfer(rewardsRecipient, balance);
+           }
        }
    }
}

Assessed type

ERC20

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L545 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L546 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L571

Vulnerability details

Unspent allowance may break functionality in AMO

An unspent allowance may cause a denial of service during the calls to safeApprove() in the AMO contract.

Impact

The AMO contract uses the safeApprove() function to grant the Curve pool permission to spend funds while adding liquidity. When adding liquidity into the Curve pool, the AMO contract needs to approve allowance so the AMM can pull tokens from the caller.

The safeApprove() function is a wrapper provided by the SafeERC20 library present in the OpenZeppelin contracts package, its implementation is the following:

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol#L45-L54

function safeApprove(IERC20 token, address spender, uint256 value) internal {
    // safeApprove should only be called when setting an initial allowance,
    // or when resetting it to zero. To increase and decrease it, use
    // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
    require(
        (value == 0) || (token.allowance(address(this), spender) == 0),
        "SafeERC20: approve from non-zero to non-zero allowance"
    );
    _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
}

As the comment warns, this should only be used when setting an initial balance or resetting it to zero. In the AMO contract the use of safeApprove() is included in the functions that are in charge of adding liquidity to the Curve pool (addLiquidity() and addLiquidityOnlyStETH()), implying a repeatedly use whenever the allowance needs to be set so that the pool can pull the funds. As we can see in the implementation, if the current allowance is not zero the function will revert.

This means that any unspent allowance of xETH or stETH (i.e. allowance(AMO, curvePool) > 0) will cause a denial of service in the addLiquidity() and addLiquidityOnlyStETH() functions, potentially bricking the contract.

Proof of concept

1. Suppose there is an unspent allowance of stETH in the AMO contract, stETH.allowance(AMO, curvePool) > 0.
2. Admin calls addLiquidityOnlyStETH(stETHAmount, minLpOut) with stETHAmount > 0 to provide liquidity to the pool.
3. Transaction will be reverted in the call to safeApprove() as (value == 0) || (token.allowance(address(this), spender) == 0) will be false.

Recommendation

Simply use approve(), or first reset the allowance to zero using safeApprove(spender, 0), or use safeIncreaseAllowance().

Note from warden

Even though the deprecated usage of safeApprove() is mentioned in the automated findings, this report demonstrates how this function can cause a serious vulnerability that may end up bricking the contract.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L96

Vulnerability details

wxETH is vulnerable to the inflation attack

The wxETH contract is vulnerable to the attack known as "inflation attack" in which a bad actor can front-run initial stake transactions and steal all deposit funds.

Impact

The staking functionality of wxETH is vulnerable to the inflation attack. This issue allows a malicious actor to front-run the initial deposit in the contract to steal all funds.

The attack, which is described in detail here and here, involves inflating the value of a share by donating assets to the pool. The attacker front-runs the initial stake transaction by first minting a single share and donating such an amount of assets that will make the front-runned deposit to mint zero shares:

1. Assume contract is empty and user wants to stake X amount of xETH.
2. Attacker front-runs transaction and:
   • Stakes 1 wei of xETH so they get 1 share of wxETH
   • Transfers (donates) X amount of xETH directly to the wxETH contract. This increases the reserves, inflating the value of a share.
3. Front-runned transaction gets executed. User stakes X amount and since the pool has X+1 assets, the user gets zero shares: shares = assets * supply / totalAssets = X * 1 / (X+1) = 0.
4. Attacker unstakes their single share. As the total supply of shares is still 1, the attacker basically owns the total amount of assets and steals the user funds.

This is a common issue present in vaults in which the underlying asset balance can be manipulated. As there is no slippage check on the deposit, the attacker is able to inflate the value of a share in order to devalue the front-runned deposit. Due to rounding issues, the original transaction is minted zero shares, which allows the attacker to control all shares of wxETH and to withdraw all the xETH balance, effectively stealing the funds from all initial deposits to the contract.

Proof of concept

In the following test, Alice wants to deposit 1e18 tokens of xETH and is front-runned by Bob, who first mints 1 share using 1 wei of xETH and then donates an equal amount of tokens as Alice to the wxETH contract. Alice's transaction is then executed and she is minted zero shares. Bob now unstakes his share to recover his deposit, along with all funds from Alice's deposit.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_wxETH_InflationAttack() public {
    // Alice will stake, Bob will play the attacker role
    uint256 depositAmount = 1e18;

    deal(address(xETH), alice, depositAmount);
    deal(address(xETH), bob, depositAmount + 1);

    // Bob frontruns Alice initial stake
    vm.startPrank(bob);

    xETH.approve(address(wxETH), type(uint256).max);

    // He first stakes 1 wei to have 1 share
    wxETH.stake(1);
    assertEq(wxETH.balanceOf(bob), 1);

    // He then donates depositAmount+1 to the contract
    xETH.transfer(address(wxETH), depositAmount);

    vm.stopPrank();

    // Now comes Alice stake
    vm.startPrank(alice);

    xETH.approve(address(wxETH), type(uint256).max);
    wxETH.stake(depositAmount);

    // Alice will have 0 shares
    assertEq(wxETH.balanceOf(alice), 0);

    vm.stopPrank();

    // Now Bob unstakes his share to steal everything
    vm.prank(bob);
    wxETH.unstake(1);

    assertEq(xETH.balanceOf(bob), depositAmount * 2 + 1);
}

Recommendation

The following discussion presents different mitigations to the attack.

As the wxETH contract doesn't conform to the ERC4626 standard, the easiest solution would be to add a slippage check on the number of minted shares. A minSharesOut parameter can be added to revert the operation if the minted shares of wxETH is below this limit, preventing the front-running.

Alternatives are minting an initial amount of dead shares so the attack becomes economically infeasible, or implementing internal accounting for the balance of the asset (xETH).

References

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/audits/2022-10-ERC4626.pdf
• https://www.rileyholterhus.com/writing/bunni
• https://mixbytes.io/blog/overview-of-the-inflation-attack

Assessed type

MEV

Report

• Low Issues (11)
• Informational Issues (2)
• Non-Critical Issues (3)

Low Issues

<a name="L-1"></a>[L-1] Wrong function declaration in IBaseRewardPool interface

The withdraw function declaration is missing the bool return value.

https://github.com/code-423n4/2023-05-xeth/blob/main/src/interfaces/IBaseRewardPool.sol#L14

function withdraw(uint256 amount, bool claim) external;

<a name="L-2"></a>[L-2] Incorrect function declaration in ICurveFactory interface

The deploy_pool function doesn't exist in the Curve Factory contract (see https://github.com/curvefi/curve-factory/blob/master/contracts/Factory.vy).

https://github.com/code-423n4/2023-05-xeth/blob/main/src/interfaces/ICurveFactory.sol#L5-L18

function deploy_pool(
    string memory name,
    string memory symbol,
    address[] memory coins,
    uint256 A,
    uint256 mid_fee,
    uint256 out_fee,
    uint256 allowed_extra_profit,
    uint256 fee_gamma,
    uint256 adjustment_step,
    uint256 admin_fee,
    uint256 ma_half_time,
    uint256 initial_price
) external returns (address);

<a name="L-3"></a>[L-3] grantRole operation in setAMO function may incorrectly check role access

https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L54

Instead of using the internal _grantRole function, the setAMO function present in the xETH is using the grantRole function of the OpenZeppelin AccessControl library, which includes a validation that the caller has the admin role for the role being granted (MINTER_ROLE in this case):

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L145-L147

function grantRole(bytes32 role, address account) public virtual override onlyRole(getRoleAdmin(role)) {
    _grantRole(role, account);
}

This should be ok as long as the admin role for MINTER_ROLE is not defined. Currently, the setAMO function will be called by the DEFAULT_ADMIN_ROLE which is the default admin role for the MINTER_ROLE role. But if the role admin for the MINTER_ROLE is defined, then the default admin role will not be the admin role for the MINTER_ROLE and the operation will be reverted.

The recommendation is to change grantRole for the internal variant _grantRole.

<a name="L-4"></a>[L-4] Potential accidental inclusion of ERC20Burnable in xETH token

https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L9

The xETH token inherits from ERC20Burnable, which defines public methods to allow burning of tokens.

Presumably this was added accidentally as a mistake in order to implement the burnShares() function in xETH. The internal function _burn is already provided in the base implementation of ERC20 (OpenZepplin library), and doesn't need any extra implementation, which may increase the attack surface of the protocol.

The recommendation is to remove ERC20Burnable from the inheritance list of xETH.

<a name="L-5"></a>[L-5] addLockedFunds should check dripRatePerBlock is not zero

https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L146-L148

The addLockedFunds function present in the wxETH contract should check that dripRatePerBlock > 0.

<a name="L-6"></a>[L-6] Use Ownable2Step instead of Ownable for access control

• https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L9
• https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L11

Use the Ownable2Step variant of the Ownable contract to better safeguard against accidental transfers of access control.

<a name="L-7"></a>[L-7] recoverToken function should restrict which tokens are allowed to be recovered

https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L101-L109

The CVXStaker contract manages different tokens as part of its natural intended behavior, notably these are the Curve LP tokens, Convex tokens, and the different reward tokens associated with the Convex staking pool.

The recoverToken implementation can arbitrarily transfer any ERC20 token, which may also be used to transfer these tokens.

The recommendation is to restrict these tokens, which are an important part of the process around the contract, and should not be "recovered" as part of an accidental side effect.

<a name="L-8"></a>[L-8] setOperator and setRewardsRecipient don't check for address(0)

• https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L78
• https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L89

Check that the arguments are not address(0).

<a name="L-9"></a>[L-9] Initialize rebalanceUpCap and rebalanceDownCap in AMO constructor

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L175

Initialize these two variables to comply with the invariant of these values being different from zero, which is checked in their respectives setters.

<a name="L-10"></a>[L-10] Validate argument in setRebalanceUpThreshold and setRebalanceDownThreshold

• https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L495
• https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L512

Validate that the thresholds are within the 0-1e18 range in setRebalanceUpThreshold and setRebalanceDownThreshold function of the AMO contract.

<a name="L-11"></a>[L-11] rebalanceUpCap and rebalanceDownCap operate on different types of value

Caps operate on different types of value. rebalanceUpCap is checked against LP tokens:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L252

if (quote.lpBurn > rebalanceUpCap) revert RebalanceUpCapExceeded();

While rebalanceDownCap is checked against xETH tokens:

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L299

if (quote.xETHAmount > rebalanceDownCap)
    revert RebalanceDownCapExceeded();

Informational Issues

<a name="INFO-1"></a>[INFO-1] Type of Curve pool is an important and sensible parameter

Protocol owners will deploy a Curve pool as an essential piece of the protocol. The current intention is to use a Curve for the pair xETH/stETH.

The protocol team has asserted that they will be using a plain pool, "balances" type, in which none of the coins are native ETH.

There are two issues that shouldn't be a problem as long as these conditions are maintained:

1. stETH being a rebasing token needs the Curve pool to be of "balances" type. Metapools are not supported either.
2. As the protocol uses Curve's get_virtual_price() (in the AMO contract) it can potentially be vulnerable to the read-only reentrancy attacked described in the following article. This attack is not possible as long as none of the coins is ETH, which is the main driver to trigger the reentrancy.

If conditions are changed, and the type of Curve pool is changed, these two potential issues could be manifested as vulnerabilities in the protocol.

<a name="INFO-2"></a>[INFO-2] Several centralization risks introduce multiple points of failure

Although much care has been taken to contain and bound the accessible functionality of the defender role, there are multiple centralization risks around the protocol that imply a lot of trust in owners or controllers of the protocol.

Here is a non-exhaustive list of the different places that may introduce potential failures due to centralization:

• xETH can be paused, freezing all operations.
• AMO can be changed in xETH, which grants arbitrary minting capabilities to the new account.
• Minting and burning is also accessible to the DEFAULT_ADMIN_ROLE.
• Configuration for wxETH rewards can be arbitrarily changed.
• Rewards for wxETH are handled manually via off-chain processes (claim revenue and call to add locked funds).
• Rebalance operations have cooldown, but cooldown can be changed to zero.
• Rebalance can be neutralized by setting zero caps.
• Rebalance thresholds can be arbitrarily changed to any amount.
• Remove liquidity functions in AMO contract can be used to empty the Curve pool.
• CXVStaker can be changed in AMO contract (sensible because this contract controls Curve LPs).
• LP tokens in CVXStaker can be withdrawn and sent to an arbitrary account (withdrawAndUnwrap).

Non Critical Issues

<a name="NC-1"></a>[NC-1] Import declarations should import specific symbols

Prefer import declarations that specify the symbol(s) using the form import {SYMBOL} from "SomeContract.sol" rather than importing the whole file

Instances (20):

File: src/AMO2.sol

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

5: import "@openzeppelin-contracts/token/ERC20/utils/SafeERC20.sol";

6: import "@openzeppelin-contracts/access/AccessControl.sol";

7: import "./interfaces/ICurvePool.sol";

File: src/CVXStaker.sol

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

5: import "@openzeppelin-contracts/token/ERC20/utils/SafeERC20.sol";

6: import "@openzeppelin-contracts/access/Ownable.sol";

7: import "./interfaces/ICurvePool.sol";

8: import "./interfaces/ICVXBooster.sol";

9: import "./interfaces/IBaseRewardPool.sol";

File: src/interfaces/IXETH.sol

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

File: src/wxETH.sol

3: import "@openzeppelin-contracts/token/ERC20/ERC20.sol";

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

5: import "@openzeppelin-contracts/token/ERC20/utils/SafeERC20.sol";

6: import "@openzeppelin-contracts/access/Ownable.sol";

7: import "solmate/utils/FixedPointMathLib.sol";

File: src/xETH.sol

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

5: import "@openzeppelin-contracts/token/ERC20/extensions/ERC20Burnable.sol";

6: import "@openzeppelin-contracts/security/Pausable.sol";

7: import "@openzeppelin-contracts/access/AccessControl.sol";

<a name="NC-2"></a>[NC-2] Use uint256 instead of the uint alias

Prefer using the uint256 type definition over its uint alias.

Instances (2):

File: src/CVXStaker.sol

195:             for (uint i = 0; i < rewardTokens.length; i++) {

File: src/wxETH.sol

16:     event UpdateDripRate(uint oldDripRatePerBlock, uint256 newDripRatePerBlock);

<a name="NC-3"></a>[NC-3] The accrueDrip() function can use _accrueDrip() to avoid the empty block implementation

The implementation of the accrueDrip() function can be changed to use _accrueDrip() instead of the drip modifier to avoid the empty block implementation.

function accrueDrip() external {
  _accrueDrip();
}

xETH.sol contract

• grantRole call in setAMO function can be changed to use the internal variant _grantRole to avoid re-checking for access control, saving gas.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L54

• The curveAMO storage variable is read 3 times from storage in setAMO. Consider using a local variable to save gas.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L44
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L45
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L54

• mintShares function visibility can be changed to external as it isn't used internally in the contract.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L63

• burnShares function visibility can be changed to external as it isn't used internally in the contract.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L75

• whenNotPaused modifier can be removed from the mintShares function as this check is already provided by the internal _beforeTokenTransfer callback.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L63

• whenNotPaused modifier can be removed from the burnShares function as this check is already provided by the internal _beforeTokenTransfer callback.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L75

• whenNotPaused modifier can be removed from the pause function as this check is already included in the internal _pause function.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L86

• whenNotPaused modifier can be removed from the unpause function as this check is already included in the internal _unpause function.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/xETH.sol#L93

wxETH.sol contract

• The SafeERC20 library can be safely removed as the contract will only deal with the xETH token, which is a known implementation that adheres correctly to the ERC20 standard.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L10

• lastReport and dripEnabled storage variables can be combined to use a single slot and save gas by decreasing the precision of lastReport to uint248, which should still be plenty enough as this represents a block number.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L60-L62

• lastReport variable initialization can be safely skipped in the constructor, as it will be eventually initialized by the call to startDrip() (dripping is disabled at start).
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L73

• lockedFunds storage variable is re-read from storage while emitting the event in the addLockedFunds() function. Consider using a local variable cache.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L156

• drip is unneeded in the function startDrip() as it is presumed that currently dripping is disabled, making _accrueDrip have no effect.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L170

• Updating lastReport in the stopDrip() is not needed as it is already updated during the call to the drip modifier.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L191

• lockedFunds storage variable is read from storage 5 times during the course of the _accrueDrip() function. Consider using local variable variants.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L236
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L241
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L248
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L254

• Calculation to update lockedFunds can be done using unchecked math since dripAmount is guaranteed to be not greater than lockedFunds.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/wxETH.sol#L241

CVXStaker.sol contract

• rewardsRecipient is re-read multiple times from storage (once per each iteration of the loop) in the getRewards() function. Consider reading it once and using a local variable after.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L190
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L195

• rewardTokens[i] is read twice in each iteration of the loop in the getRewards() function. Consider reading it once and using a local variable after.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L192 https://github.com/code-423n4/2023-05-xeth/blob/main/src/CVXStaker.sol#L195

AMO2.sol contract

• The SafeERC20 library can be safely removed as the contract will only deal with xETH and stETH tokens, which are known implementations that adheres correctly to the ERC20 standard.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L12

• Infinite token approval can be granted to the curve pool for the xETH and stETH tokens at contract construction time. This will save the approval calls present in each of the functions that deal with adding liquidity to the pool.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L309
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L545
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L546
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L571

• rebalanceUp function can take struct parameter from calldata instead of memory, saving a copy operation.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L240

• rebalanceDown function can take struct parameter from calldata instead of memory, saving a copy operation.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L287

• bestRebalanceUpQuote function can take struct parameter from calldata instead of memory, saving a copy operation.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L336

• bestRebalanceDownQuote function can take struct parameter from calldata instead of memory, saving a copy operation.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L361

• rebalanceUp function reads the cvxStaker twice from storage. Consider reading it once and using a local variable after. https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L256
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L261

• rebalanceDown function reads the cvxStaker twice from storage. Consider reading it once and using a local variable after. https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L314
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L317

• bestRebalanceUpQuote and bestRebalanceDownQuote can return a single result (min_xETHReceived and minLpReceived respectively) instead of returning a whole new struct in memory.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L337
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L362

• defender storage variable is read 3 times from storage in the implementation of setRebalanceDefender. Consider using a local variable variant.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L391
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L392
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L395

• minAmounts array is not needed in the removeLiquidityOnlyStETH() function and can be safely removed.
  https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L640-L642