valuevalk - Protocol's interestFees + Interest in a pool can be lost because of precision loss when using low-decimal assets like USDT/USDC.

[sherlock-admin2]

valuevalk

High

Protocol's interestFees + Interest in a pool can be lost because of precision loss when using low-decimal assets like USDT/USDC.

Summary

Lenders/Borrowers can intentionally lend/borrow low amounts of assets in short periods of time to avoid "paying the protocol" the interestFee, when using 6 decimal asset such as USDT/USDC.

Those issues could also happen unintentionally if there is just a constant volume of transactions for relatively low amounts.

Vulnerability Detail

Lenders could also benefit as the feeShares are not minted and added to the Pool.totalDepositShares, thus the shares of the Lenders keep their value.

Borrower's ability can be limited to perform the attack, if the Pool.minBorrow amount is set high enough, though precision loss could still occur. BUT, Lenders do not have such limitations.

Additionally, its also likely to have precision loss in the whole InterestAccrued itself, which benefits Borrowers, at the expense of Lenders.

Impact

The protocol continuously loses the interestFee, due to precision loss.

Lenders could do this in bulk with low-value amounts when borrowing to avoid fees to the protocol when depositing.

If minBorrow is set low enough Borrowers can intentionally do it too.

Since the protocol would be deployed on other EVM-compatible chains, the impact would be negligible when performed on L2s and potentially on Ethereum if gas fees are low.

The losses could be significant, when compounding overtime.

Code Snippet

accrue() is called in deposit() and borrow()
And it saves the pool.lastUpdated , every time its called.

    function accrue(PoolData storage pool, uint256 id) internal {
        (uint256 interestAccrued, uint256 feeShares) = simulateAccrue(pool);
        if (feeShares != 0) _mint(feeRecipient, id, feeShares);
        
        pool.totalDepositShares += feeShares;
        pool.totalBorrowAssets += interestAccrued;
        pool.totalDepositAssets += interestAccrued;

        // store a timestamp for this accrue() call
        // used to compute the pending interest next time accrue() is called
 @>>    pool.lastUpdated = uint128(block.timestamp);
    }

So upon the next call we will use that timestamp in the simulateAccrue().
However interestAccrued.mulDiv(pool.interestFee, 1e18); loses precision when using low-decimal assets such as USDT/USDC, and if its called within short period of times like 1-60 seconds it can even end up being 0, and since pool.lastUpdated is updated, the lost amounts cannot be recovered the next time accrue() is called.

    function simulateAccrue(PoolData storage pool) internal view returns (uint256, uint256) {
@>>     uint256 interestAccrued = IRateModel(pool.rateModel).getInterestAccrued(
@>>         pool.lastUpdated, pool.totalBorrowAssets, pool.totalDepositAssets
        );

        uint256 interestFee = pool.interestFee;
        if (interestFee == 0) return (interestAccrued, 0);
@>>     uint256 feeAssets = interestAccrued.mulDiv(pool.interestFee, 1e18);
        
       .........

Proof of Concept

In BaseTest.t.sol set interestFee to 10% of the Interest.

            badDebtLiquidationDiscount: 1e16,
            defaultOriginationFee: 0,
-           defaultInterestFee: 0
+          defaultInterestFee: 0.1e18
        });

and make asset1 have 6 decimals, as USDT/USDC

-       asset1 = new MockERC20("Asset1", "ASSET1", 18);
+      asset1 = new MockERC20("Asset1", "ASSET1", 6);
        asset2 = new MockERC20("Asset2", "ASSET2", 18);
        asset3 = new MockERC20("Asset3", "ASSET3", 18);

Changes in PositionManager.t.sol

        vm.startPrank(protocolOwner);
        riskEngine.setOracle(address(asset1), address(asset1Oracle));
        riskEngine.setOracle(address(asset2), address(asset2Oracle));
        riskEngine.setOracle(address(asset3), address(asset3Oracle));
        vm.stopPrank();

-       asset1.mint(address(this), 10_000 ether);
-       asset1.approve(address(pool), 10_000 ether);
+       asset1.mint(address(this), 10_000e6);
+       asset1.approve(address(pool), 10_000e6);

-       pool.deposit(linearRatePool, 10_000 ether, address(0x9));
+       pool.deposit(fixedRatePool2, 10_000e6, address(0x9));

        Action[] memory actions = new Action[](1);
        (position, actions[0]) = newPosition(positionOwner, bytes32(uint256(3_492_932_942)));

        PositionManager(positionManager).processBatch(position, actions);

        vm.startPrank(poolOwner);
       riskEngine.requestLtvUpdate(linearRatePool, address(asset3), 0.75e18);
       riskEngine.acceptLtvUpdate(linearRatePool, address(asset3));
-       riskEngine.requestLtvUpdate(linearRatePool, address(asset2), 0.75e18);
-       riskEngine.acceptLtvUpdate(linearRatePool, address(asset2));
+       riskEngine.requestLtvUpdate(fixedRatePool2, address(asset2), 0.75e18);
+       riskEngine.acceptLtvUpdate(fixedRatePool2, address(asset2));
        vm.stopPrank();

Add the code bellow in the PositionManager.t.sol and run forge test --match-test testZeroFeesPaid -vvv

    function testZeroFeesPaid() public {
        //===> Assert 0 total borrows <===
        assertEq(pool.getTotalBorrows(fixedRatePool2), 0);

        //===> Borrow asset1 <===
        testSimpleDepositCollateral(1000 ether);
        borrowFromFixedRatePool();
        assertEq(pool.getTotalBorrows(fixedRatePool2), 5e6); // Borrow 5 USDT ( can be more, but delay has to be lower )

        //===> Skip 45 seconds of time, and borrow again, to call accrue and mint feeShares. <===
        skip(45);
        //Note: This could also be done using deposit (i.e. from Lenders), since we only care about the accrue function.
        borrowFromFixedRatePool();

        // Verify that feeShares minted are 0. So we lost fees between the two borrows.
        assertEq(pool.getAssetsOf(fixedRatePool2, address(this)), 0);

        //===> Try longer period low amounts of feeInterest should accrue. <===
        skip(300);
        borrowFromFixedRatePool();
        assertEq(pool.getAssetsOf(fixedRatePool2, address(this)), 18);
    }

    function borrowFromFixedRatePool() public {
        vm.startPrank(positionOwner);
        bytes memory data = abi.encode(fixedRatePool2, 5e6);

        Action memory action = Action({ op: Operation.Borrow, data: data });
        Action[] memory actions = new Action[](1);
        actions[0] = action;
        PositionManager(positionManager).processBatch(position, actions);
    }

Tool used

Manual Review

Recommendation

The core cause is that the RateModels when accounting for low-decimal assets, for short-periods of time they return low values which leads to 0 interestFee.

A possible solution to fix that would be to scale up the totalBorrowAssets and totalDepositAssets to always have 18 decimals, no matter the asset.

Thus avoiding uint256 feeAssets = interestAccrued.mulDiv(pool.interestFee, 1e18); resuling in 0, due to low interestAccrued.

This will also fix possible precision loss from interestAccrued itself, as we could also lose precision in the RateModels, which could compound and result in getting less interest, than it should be.

Additionally, consider adding a minimum deposit value.

[MrValioBg]

This issue is valid. The PoC shows the vulnerability. @z3s

We set the interest fee to 10% - as specified in the readme, admin will set it from 0 to 10, so its a valid value, the highest one is used, to show the biggest impact - reference

The problem arises from the loss of precision when using USDT/USDC.
If there is low totalBorrowedAmount, about 5-10USDT/USDC
The first interactions with the pool that result in calling simulateAccrue() could lead to 100% loss of the interestFees, as demonstrated in the PoC.

        //===> Skip 45 seconds of time, and borrow again, to call accrue and mint feeShares. <===
        skip(45);
        //Note: This could also be done using deposit (i.e. from Lenders), since we only care about the accrue function.
        borrowFromFixedRatePool();
        
         // Verify that feeShares minted are 0. So we lost fees between the two borrows.
        assertEq(pool.getAssetsOf(fixedRatePool2, address(this)), 0);

As you can see, for 45 seconds, no fee is accrued due to the precision loss, but when you call accrue, you actually save a "checkpoint," which basically leads to resetting the point from where the fees accrue.
Thus, it will mean that the frequent interactions could lead to the interestFees to be 100% lost, as there won't be enough time for the precision loss to be reduced, rounding them to 0.

---

Additional info:
If we set the interestFee to 2%, which is a possible value according to the Readme, it will take over 250 seconds, to get to over the rounding to 0. ( for 1%, which is also a valid value, it will take about 8-10 mins).

We would still lose great amount of fee due to precision loss, even if the pool borrow size increases, it would just be less than 100% loss, 100% might still be possible, but the time delay needed may be reduced.

For example with a pool with 1K borrow amount, it takes around 3sec to delay to round down the interestFee to 0, over the 3 sec mark, precision is still lost, and less interestFees are accrued, its just less than 100%

[ZdravkoHr]

Escalate
On behalf of @MrValioBg

[sherlock-admin3]

Escalate
On behalf of @MrValioBg

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[cvetanovv]

@z3s @ruvaag can I have your opinion?

[cvetanovv]

@MrValioBg The PoC test only works at low borrow values. As soon as I tried using larger values, it did not work. Can you provide a working PoC test with normal values?
Because precision lost on a borrow of 5 USDT is no more than low/info severity.

[MrValioBg]

@MrValioBg The PoC test only works at low borrow values. As soon as I tried using larger values, it did not work. Can you provide a working PoC test with normal values? Because precision lost on a borrow of 5 USDT is no more than low/info severity.

Of course. Will get back to you with a realistic scenario which also reflects realistic market interest rates.

[MrValioBg]

@cvetanovv
Giving a realistic example. The issue here is valid for pools with borrow amounts which is in the thousands, they can lose very high % of the interestFee, due to precision loss when using USDC/USDT.

My last PoC used unrealistic interest rate, as we used fixedPoolRate2 which had 2e18 RATE set representing 200% interest rate per year.
A realistic one is like 1-3% per year, as we can see what the competitive market offers:
https://www.explorer.euler.finance/
https://app.euler.finance/
https://app.aave.com/markets/

A pool with a realistic total borrows amount is 2000$, on such pools we could realistically have about 5-10k$ of liquidity deposited.

Setting the interestFee to 1% ( reference ) in BaseTest.t.sol

    function setUp() public virtual {
        Deploy.DeployParams memory params = Deploy.DeployParams({
            owner: protocolOwner,
            proxyAdmin: proxyAdmin,
            feeRecipient: address(this),
            minLtv: 2e17, // 0.1
            maxLtv: 8e17, // 0.8
            minDebt: 0,
            minBorrow: 0,
            liquidationFee: 0,
            liquidationDiscount: 200_000_000_000_000_000,
            badDebtLiquidationDiscount: 1e16,
            defaultOriginationFee: 0,
-           defaultInterestFee: 0
+           defaultInterestFee: 0.01e18
        });

And in BaseTest.t.sol, again set the annual interest rate for borrowers to 1.5% ( Previous value of 200% is not realistic to the market )

        address fixedRateModel = address(new FixedRateModel(1e18));
        address linearRateModel = address(new LinearRateModel(1e18, 2e18));
-       address fixedRateModel2 = address(new FixedRateModel(200e18)); 
+       address fixedRateModel2 = address(new FixedRateModel(0.015e18)); // set interest rate to 1.5%
        address linearRateModel2 = address(new LinearRateModel(2e18, 3e18));

Additionally we still need to do the changes in setUp() and in BaseTest.t.sol from the original PoC, which are related to the 6 decimal change, which comes from using either USDT or USDC.
Then we can run the adjusted PoC:

    function testZeroFeesPaid() public {
        //===> Assert 0 total borrows <===
        assertEq(pool.getTotalBorrows(fixedRatePool2), 0);

        //===> Setup <===
        testSimpleDepositCollateral(200_000 ether);
        borrowFromFixedRatePool(2000e6);
        assertEq(pool.getTotalBorrows(fixedRatePool2), 2000e6); // TotalBorrows from pool are 2000$
        //-------------------

        //===> Skip 3.5 minutes, and borrow again, to call accrue and mint feeShares. <===
        skip(205);
        //Note: This could also be done using deposit (i.e. from Lenders), since we only care about the accrue function.
        borrowFromFixedRatePool(1e6); //Someone borrows whatever $$ from the pool, so accrue can be called.

        // Verify that feeShares minted are 0. So we lost 100% of the fees between the two borrows.
        assertEq(pool.getAssetsOf(fixedRatePool2, address(this)), 0);

        //===> Try longer period - 40 minutes <===
        skip(60 * 40);
        borrowFromFixedRatePool(5e6); // again borrow whatever $$ from the pool, so accrue can be called.

        // Very small amount of fee accrued, due to precision loss.
        // Even though its not 100% loss, its still high loss.
        assertEq(pool.getAssetsOf(fixedRatePool2, address(this)), 21);

        //================================================================================================
        //Now lets compare what should the actual acrued fee would have looked like for this time period.

        //Retrieve accrued interest
        FixedRateModel rateModelOfPool = FixedRateModel(pool.getRateModelFor(fixedRatePool2)); // get the rate model of
        uint256 timeStampLastUpdated = block.timestamp - 60 * 40; // 12 hours period
        uint256 scaledTotalBorrows = pool.getTotalBorrows(fixedRatePool2) * 1e12; // scale to 18 decimals
        uint256 interestAccrued = rateModelOfPool.getInterestAccrued(
            timeStampLastUpdated, scaledTotalBorrows, pool.getTotalAssets(fixedRatePool2)
        );

        //Calculate % loss of fee, for 25 minutes delay.
        (,,,,, uint256 poolInterestFee,,,,,) = pool.poolDataFor(linearRatePool);
        uint256 feeReal = interestAccrued * poolInterestFee / 1e18;

        assertEq(feeReal, 22_883_897_764_107);

        //21 is fee with lost precision, scale up to 18 decimals and compare with the real fee.
        // 22883897764107 / 21000000000000 = 1.0897 or 9% loss of fees.
    }

The main constraint we have here is the required activity of the pool. Such time delays of 40 minutes for 9% loss, as shown in the PoC are realistic, However an arbitrary user can also just do frequent deposits and lend asset, he just needs to do it once every 40 minutes to cost the protocol 9% of the interestFee.
I also tested with 6hrs delay for the same pool and it still leads to more than 1% loss of the fees.

One transaction for deposit costs around 125k gas, which is about 0.005$ to 0.01$ on polygon( this transaction with 280k gas costs < 0.01$) , which will cost just 65-75$/year to maintain 9% loss by doing interaction every 40 mins.

Additionally using accrue() directly costs only 33k gas, which means that for 1 year to sustain 9% loss every time accrue() is called, it would cost just 15$. To completely round down fees to 0 and have 100% loss it will be around 160$/year.

Note: If a single attack can cause a 0.01% loss but can be replayed indefinitely, it will be considered a 100% loss and can be medium or high, depending on the constraints.

We can also call accrue directly for multiple pools, which will scale the attack for multiple pools, for cheaper pricing per pool, as we will save intrinsic gas costs.
Only 270k gas for 25 pools:

This can be done for cheaper as well, just by depositing very small amounts every time, as there isn't minimum deposit amount, as with borrowing. Since he is lending and providing liquidity attacker will just get back the gas fees lost from the yield earned.

Also since we have losses even on calling accrue() with hours delay it is reasonable to consider that this could happen from normal interactions, as well.

I consider and marked this issue HIGH since the losses can greatly exceed 1% and can be reproduced idenfinetely, in some cases causing even directly 100% loss of fees. Having one interaction every few hours without minimum amount of deposit is not huge constraint and can also occur naturally.

[cvetanovv]

@MrValioBg This PoC test not work. You forgot to show borrowFromFixedRatePool(). If possible add the console.log to see what the losses are.

btw most protocols use the same implementation of accrue() when it needs to accrue interest. I don't see why there would be an issue here.

[MrValioBg]

@cvetanovv
Sorry about the borrowFromFixedRatePool(), here you are:

    function borrowFromFixedRatePool(
        uint256 borrowAmount
    ) public {
        vm.startPrank(positionOwner);
        bytes memory data = abi.encode(fixedRatePool2, borrowAmount);

        Action memory action = Action({ op: Operation.Borrow, data: data });
        Action[] memory actions = new Action[](1);
        actions[0] = action;
        PositionManager(positionManager).processBatch(position, actions);
    }

The assertions & the comments show the losses.

Which protocols for example? Do they have the same interestFee variable, calculated in this way, the precision loss is in the interestFee which can round it down to 0, the precision in the interest accrued itself should not be as noticeable.

[cvetanovv]

After a thorough review, I believe this issue could be of Medium severity.

Watson shows precision loss when handling low-decimal assets like USDC/USDT, which can result in interest fees rounding down to zero in certain cases.

While the impact is more pronounced with small borrow amounts and short accrual periods, cumulative losses can still add up over time.

Planning to accept the escalation and make this issue a Medium severity.

[WangSecurity]

Result:
Medium
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ZdravkoHr: accepted