Mitigation Confirmed for M-01

[code423n4]

MITIGATION IS NOT CONFIRMED

MITIGATION IS NOT CONFIRMED

Mitigation of M-01: Issue not mitigated

Link to Issue: https://github.com/code-423n4/2023-03-asymmetry-findings/issues/1078

Comments

While the "division before multiplication" issues described in M-01 have been mitigated in the proposed changeset, there are other cases which should be addressed too.

Technical Details

• In SafEth::stake the calculation of preDepositPrice (now present in the function approxPrice()) multiplies underlyingValue by 1e18, but underlyingValue is first divided by 1e18:

https://github.com/asymmetryfinance/smart-contracts/blob/fixMath/contracts/SafEth/SafEth.sol#L356-L370

function approxPrice() public view returns (uint256) {
    uint256 safEthTotalSupply = totalSupply();
    uint256 underlyingValue = 0;
    uint256 count = derivativeCount;

    for (uint256 i = 0; i < count; i++) {
        if (!derivatives[i].enabled) continue;
        IDerivative derivative = derivatives[i].derivative;
        underlyingValue +=
            (derivative.ethPerDerivative() * derivative.balance()) /
            1e18;
    }
    if (safEthTotalSupply == 0 || underlyingValue == 0) return 1e18;
    return (1e18 * underlyingValue) / safEthTotalSupply;
}

• Also in SafEth::stake, something similar happens with totalStakeValueEth, as this is the sum of derivativeReceivedEthValue that is divided by 1e18, but then totalStakeValueEth is multiplied by 1e18 in the calculation of mintAmount:

https://github.com/asymmetryfinance/smart-contracts/blob/fixMath/contracts/SafEth/SafEth.sol#L97-L114

uint256 totalStakeValueEth = 0; // total amount of derivatives staked by user in eth
for (uint256 i = 0; i < count; i++) {
    if (!derivatives[i].enabled) continue;
    uint256 weight = derivatives[i].weight;
    if (weight == 0) continue;
    IDerivative derivative = derivatives[i].derivative;
    uint256 ethAmount = (msg.value * weight) / totalWeight;

    if (ethAmount > 0) {
        // This is slightly less than ethAmount because slippage
        uint256 depositAmount = derivative.deposit{value: ethAmount}();
        uint256 derivativeReceivedEthValue = (derivative
            .ethPerDerivative() * depositAmount) / 1e18;
        totalStakeValueEth += derivativeReceivedEthValue;
    }
}
// mintedAmount represents a percentage of the total assets in the system
mintedAmount = (totalStakeValueEth * 1e18) / preDepositPrice;

• In Reth::deposit, rethPerEth is calculated as (1e36) / ethPerDerivative() but then rethPerEth is used in the minOut calculation that ends dividing again by 1e36. The expression can be simplified to avoid any division before multiplication. The same applies to idealOut:

https://github.com/asymmetryfinance/smart-contracts/blob/fixMath/contracts/SafEth/derivatives/Reth.sol#L148-L151

uint256 rethPerEth = (1e36) / ethPerDerivative();
uint256 minOut = ((rethPerEth * msg.value) * (1e18 - maxSlippage)) /
    1e36;
uint256 idealOut = (rethPerEth * msg.value) / 1e18;

Recommendation

Fix these other cases of "division before multiplication". In most of these, the expressions can be simplified as there is a division that is later multiplied by the same number.

[elmutt]

thanks

[d3e4]

This describes both how the issue hasn't been mitigated, and a new issue. As a new issue it is a duplicate of #71, which is submitted as a new issue.

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes marked the issue as duplicate of #71

[c4-judge]

Picodes changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by Picodes