c4-bot-3 commented 8 months ago

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/MainHelper.sol#L525-L530

Vulnerability details

Impact

Low fee amounts and fee shares are calculated in the preparation part due to the precision loss.

Proof of Concept

Solidity rounds down the result of an integer division, and because of that, it is always recommended to multiply before dividing to avoid that precision loss. In the case of a prior division over multiplication, the final result may face serious precision loss as the first answer would face truncated precision and then multiplied to another integer. The problem arises in the pool's preparation part before applying the LASA algorithm. After cleaning up the pool, the next step is to update the pseudo amounts and adding the corresponding fee shares of that pool.

If we look deeply at the function _updatePseudoTotalAmounts() we can see the fee shares calculation procedure is presented as:

        uint256 amountInterest = bareIncrease
            / PRECISION_FACTOR_YEAR;

        uint256 feeAmount = amountInterest
            * globalPoolData[_poolToken].poolFee
            / PRECISION_FACTOR_E18;

we can see there is a hidden division before a multiplication that makes round down the whole expression. This is bad as the precision loss can be significant, which leads to the pool printing less feeAmount than actual. Also, it is better to mention that some protocols implement this method to have an integer part of the division (usually in time-related situations). But here we can clearly see that this pattern is used in the calculation of feeAmount at which the precision matters. Furthermore, the mentioned error will escalate, especially when the bareIncrease is bigger but close to the PRECISION_FACTOR_YEAR amount. The precision loss becomes more serious at lower discrepancies (such as 1.2 ~ 2 magnitudes of PRECISION_FACTOR_YEAR).

As the Proof of Concept part, we can check this behavior precisely.

You can run this code to see the difference between the results:

    function test_precissionLoss() public {

        uint x = (PRECISION_FACTOR_YEAR * 3)/ 2; // This number represents the `bareIncrease`
        uint256 poolFee = 789600000000000000000000; // This number represents the `poolFee`

        uint256 amountInterest = x
            / PRECISION_FACTOR_YEAR;

        uint256 feeAmount1 = amountInterest
            * poolFee
            / PRECISION_FACTOR_E18;

        uint256 feeAmount2 = (x * poolFee) / (PRECISION_FACTOR_YEAR * PRECISION_FACTOR_E18);

        console.log("Current Implementation ", feeAmount1);
        console.log("Actual Implementation ", feeAmount2);
    }

The result would be: (for 1.5 of PRECISION_FACTOR_YEAR)

     Current Implementation  789600
     Actual Implementation  1184400

Thus, we can see that the actual implementation produces less fee amount than the precise method. This test shows a big difference between the two calculated fee amounts in the LASA algorithm.

Tools Used

Manual Review + Forge

Recommended Mitigation Steps

Consider modifying the fee shares calculation to prevent such precision loss and prioritize the multiplication over division:

        uint256 amountInterest = bareIncrease
            / PRECISION_FACTOR_YEAR;

        uint256 feeAmount = bareIncrease
            * globalPoolData[_poolToken].poolFee
            / (PRECISION_FACTOR_E18 * PRECISION_FACTOR_YEAR);