Potential loss of precision in stake and unstake actions of wxETH

[code423n4]

Lines of code

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

Vulnerability details

Potential loss of precision in stake and unstake actions of wxETH

The calculations involved in the stake and unstake actions can potential suffer from precision loss, affecting the number of minted wxETH shares or the amount of redeemed xETH.

Impact

The stake() function present in the wxETH can be used to stake xETH tokens and get in return a number of wxETH tokens, that represent the user's share in the pool. The calculation of the number of shares based on the amount of assets can be found in the previewStake() and exchangeRate() functions:

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

82:     function previewStake(uint256 xETHAmount) public view returns (uint256) {
83:         /// @dev if xETHAmount is 0, revert.
84:         if (xETHAmount == 0) revert AmountZeroProvided();
85: 
86:         /// @dev calculate the amount of wxETH to mint before transfer
87:         return (xETHAmount * BASE_UNIT) / exchangeRate();
88:     }

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

202:     function exchangeRate() public view returns (uint256) {
203:         /// @dev if there are no tokens minted, return the initial exchange rate
204:         uint256 _totalSupply = totalSupply();
205:         if (_totalSupply == 0) {
206:             return INITIAL_EXCHANGE_RATE;
207:         }
208: 
209:         /// @dev calculate the cash on hand by removing locked funds from the total xETH balance
210:         /// @notice this balanceOf call will include any lockedFunds,
211:         /// @notice as the locked funds are also in xETH
212:         uint256 cashMinusLocked = xETH.balanceOf(address(this)) - lockedFunds;
213: 
214:         /// @dev return the exchange rate by dividing the cash on hand by the total supply
215:         return (cashMinusLocked * BASE_UNIT) / _totalSupply;
216:     }

As we can see in the previous snippets, previewStake() will basically divide the amount of assets by the exchange rate, but the exchange rate is also a division of the total assets (cashMinusLocked) by the total supply of the shares (_totalSupply). Focusing on the general case when _totalSupply > 0, the calculation can be summarized as:

shares = (xETHAmount * BASE_UNIT) / ((cashMinusLocked * BASE_UNIT) / _totalSupply)

We have a division where the denominator is also a division, which implies a potential loss of precision.

Similarly, in the unstake() function, previewUnstake() is used to calculate the redeemed amount of xETH given a number of wxETH shares:

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

114:     function previewUnstake(uint256 wxETHAmount) public view returns (uint256) {
115:         /// @dev if wxETHAmount is 0, revert.
116:         if (wxETHAmount == 0) revert AmountZeroProvided();
117: 
118:         /// @dev calculate the amount of xETH to return
119:         return (wxETHAmount * exchangeRate()) / BASE_UNIT;
120:     }

Expanding the calculation, we have:

amount = (wxETHAmount * ((cashMinusLocked * BASE_UNIT) / _totalSupply)) / BASE_UNIT

Arriving again at a similar case of potential loss of precision.

Recommendation

The calculations can be simplified in order to avoid intermediate divisions and loss of precision. For staking, previewStake() can be refactored as:

function previewStake(uint256 xETHAmount) public view returns (uint256) {
    /// @dev if xETHAmount is 0, revert.
    if (xETHAmount == 0) revert AmountZeroProvided();

    uint256 _totalSupply = totalSupply();

    if (_totalSupply == 0) {
      return xETHAmount;
    } else {
      uint256 cashMinusLocked = xETH.balanceOf(address(this)) - lockedFunds;

      return (xETHAmount * _totalSupply) / cashMinusLocked;  
    }
}

While for unstaking, previewUnstake() can be refactored as:

function previewUnstake(uint256 wxETHAmount) public view returns (uint256) {
    /// @dev if wxETHAmount is 0, revert.
    if (wxETHAmount == 0) revert AmountZeroProvided();

    uint256 _totalSupply = totalSupply();

    /// @dev calculate the amount of xETH to return
    if (_totalSupply == 0) {
      return wxETHAmount;
    } else {
      uint256 cashMinusLocked = xETH.balanceOf(address(this)) - lockedFunds;

      return (wxETHAmount * cashMinusLocked) / _totalSupply;
    }
}

Assessed type

Decimal

[c4-judge]

kirk-baird marked the issue as primary issue

[c4-sponsor]

vaporkane marked the issue as sponsor confirmed

[c4-sponsor]

vaporkane marked the issue as disagree with severity

[kirk-baird]

This is a valid issue in that it may cause rounding errors.

However, the calculations where division are involved use BASE_UINT to increase the value of the numerator by 1e18. This minimises the impact of rounding issues to the order of Wei. As a result there is negligible impact from rounding errors and I consider this to be a low severity issue.

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[c4-judge]

kirk-baird marked the issue as grade-a

[c4-sponsor]

vaporkane marked the issue as sponsor acknowledged