Asymmetry contest - Matin's results

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L73-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L112 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L215

Vulnerability details

Impact

Precision loss due to wrong mathematical operation orders.

Proof of Concept

The function stake() is intended to get the ETH of a user and convert it to the derivative tokens which are currently three staked ERC20 tokens. This function gets the ETH value of the derivatives mentioned above and calculates the amount needed for minting the SafETH upgradeable token. The function ethPerDerivative() is responsible for getting the price of a derivative in terms of ETH, which is implemented in each derivative contract. Considering the price calculation of the derivatives inside the ethPerDerivative() is correct (which is excluded from the contest scope), the WstEth works correctly. The Reth also seems not to have a problem at this stage. However, the staked Frax Ether (SfrxEth) performs a convertToAssets() in its core which has a mulDivDown() from the fixed-point math library, and then performs a division over the oracle-based price. Thus it suffers from a precision loss.

mulDivDown(x,y,z) is simply equal to $\dfrac{xy}{z}$ at which the denominator is totalSupply in convertToAssets() function:

    function convertToAssets(uint256 shares) public view virtual returns (uint256) {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

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

Thus, the return value of this function is multiplied by $10^{18}$ and then divided by the price. In the next step, this value is multiplied by the balance of each derivative in the contract SafETH:

            (derivatives[i].ethPerDerivative(derivatives[i].balance()) *
                derivatives[i].balance()) /
                10 ** 18;

In simple words, the code wants to perform these tasks:

$ a = \dfrac{10^{18} \times totalAssets}{totalSupply} $

$ b = \dfrac{10^{18} \times a}{priceOracle} $

$ c = \dfrac{b \times balance}{10^{18}} $

To implement these steps through solidity, we need to perform all the multiplications and then divide the results to avoid precision loss. To test this precision loss, a simple foundry test code may be implemented:

    function testDivision(uint _totalSupply, uint totalAssets, uint balance) external {

        uint256 supply = _totalSupply;
        uint256 shares = 1e18;
        uint256 oracle = 1.03e18; // SFrxETH approximate price
        uint256 convertToAssets = shares.mulDivDown(totalAssets, supply);
        uint256 classicDivision = ((1e18 * division / oracle) * balance) / 1e18;

        uint256 numerator = 1e18 * shares * totalAssets * balance;
        uint256 denominator = oracle * supply * 1e18;
        uint256 correctDivision = numerator/denominator;

        assertFalse(correctDivison == classicDivision);

    }

This test fails for arbitrary totalSupply, totalAssets, and balance.

Thus, the underlyingValue is calculated wrongly.

Tools Used

Manual analysis, Foundry

Recommended Mitigation Steps

It's recommended the protocol avoid division before multiplication and perform division operations at last.