ShadowForce - division before multiplication may result in truncation of result

[sherlock-admin]

ShadowForce

high

division before multiplication may result in truncation of result

Summary

division before multiplication may result in truncation of result

Vulnerability Detail

 function _calculateMinRepayUnits(uint256 _collateralRebalanceUnits, uint256 _slippageTolerance, ActionInfo memory _actionInfo) internal pure returns (uint256) {
        // @audit
        // division before manipulation?
        return _collateralRebalanceUnits
            .preciseMul(_actionInfo.collateralPrice)
            .preciseDiv(_actionInfo.borrowPrice)
            .preciseMul(PreciseUnitMath.preciseUnit().sub(_slippageTolerance));
    }

in the snippet above we can see there is division before multiplication. As we all know doing this can result in truncation. funds may be lost (0) due to division before multiplication precision issues

for example: if _actionInfo.borrowPrice is less than 1, it will be truncated to 0. any further multiplication with this number will also result in 0. we observe multiplication after this point in this specific place.

 .preciseMul(_actionInfo.collateralPrice)
            .preciseDiv(_actionInfo.borrowPrice)
            .preciseMul(PreciseUnitMath.preciseUnit().sub(_slippageTolerance));

all multiplacation must first be done before we decide to divide.

Impact

funds may be lost (0) due to division before multiplication precision issues

Code Snippet

https://github.com/IndexCoop/index-coop-smart-contracts/blob/317dfb677e9738fc990cf69d198358065e8cb595/contracts/adapters/AaveLeverageStrategyExtension.sol#L1147-L1152

Tool used

Manual Review

Recommendation

multiply first then divide to avoid precision loss

[ckoopmann]

While technically correct the precision loss would be limited to 3/4 decimals, and will not have a material impact when applied to the "minAmountOut" parameter.

We might adjust the order anyway, but will review first internally.

Not sure about "medium" severity, since this does not seem to have a material impact on the user experience / protocol safety.

[pblivin0x]

The proposed fix to flip the order of operations here looks reasonable to me 👍

[0xffff11]

I believe sherlock does mark this issue as a med in most cases because there is a small impact in the calculation of funds

[0xffff11]

There are several instances on the code where this happens, all of them market as duplicates. I believe that a small truncation where precission would be needed due to calculating any type of repayment is a medium

[hrishibhat]

While technically correct the precision loss would be limited to 3/4 decimals, and will not have a material impact when applied to the "minAmountOut" parameter.

Considering this issue as a low

[ckoopmann]

Fixed in: https://github.com/IndexCoop/index-coop-smart-contracts/pull/142

[IAm0x52]

Fix looks good. Instances of div before mul have been swapped