Darinrikusham - Calculation issue will impact in loss in user funds and DOS

[sherlock-admin3]

Darinrikusham

High

Calculation issue will impact in loss in user funds and DOS

Summary

In withdraw function in pool.sol contract while calculating amount of shares it is rounding up which results in loss in user funds and DOS as user will not be able to fully withdraw the deposited assets and Superpool is also not compliant with ERC 4626.

Root Cause

The choice to round up on pool.sol:350 is a mistake as it results in loss of user funds and DOS as user can't withdraw the full asset amount. It also affects withdraw function in SuperPool.sol as underneath it calls withdraw function on pool.sol

Internal pre-conditions

It will happen in all scenarios after interest is accrued and peg is not 1:1 between assets and shares.

External pre-conditions

No response

Attack Path

1. In testTimeIncreasesDebt test case after time is elapsed, debt is increased and interest is accrued if a user deposits funds and later tries to withdraw all the funds they are not able to withdraw it all due to issue in calculation

Impact

The user loose a part of deposited asset amount and can't withdraw the deposited amount fully causing withdraw function unusable to withdraw all assets and also cause the same DOS issue in superPool withdraw function.

PoC

In Pool.t.sol

function testTimeIncreasesDebt(uint96 assets) public {
        testBorrowWorksAsIntended(assets);

        (,,,,,,, uint256 totalBorrowAssets, uint256 totalBorrowShares,,) = pool.poolDataFor(linearRatePool);
        console.log("totalBorrowAssets -> ", totalBorrowAssets);
        console.log("totalBorrowShares -> ", totalBorrowShares);

        uint256 time = block.timestamp + 1 days;
        vm.warp(time + 86_400 * 7);
        vm.roll(block.number + ((86_400 * 7) / 2));

        pool.accrue(linearRatePool);

        (,,,,,,, uint256 newTotalBorrowAssets, uint256 newTotalBorrowShares,,) = pool.poolDataFor(linearRatePool);
        console.log("newTotalBorrowAssets -> ", newTotalBorrowAssets);
        console.log("newTotalBorrowShares -> ", newTotalBorrowShares);
        console.log("shares first -> ", pool.balanceOf(user, linearRatePool));
        console.log("assets -> ",assets);

        (uint256 depositedEarlier) = pool.getAssetsOf(linearRatePool, user);
        console.log("assets in pool before ->", depositedEarlier);
        // (uint256 liquidity) = pool.getPoolAssetFor(linearRatePool);
        // console.log("total assets in pool ->", liquidity);

        vm.startPrank(user2);
        asset1.mint(user2, assets);
        asset1.approve(address(pool), assets);
        (uint256 sharesBefore) = pool.deposit(linearRatePool, assets, user2);
        console.log("shares minted on deposit -> ", sharesBefore);
        (uint256 sharesAfter) = pool.withdraw(linearRatePool, assets, user2, user2);
        console.log("shares burned on withdraw -> ", sharesAfter);
        vm.stopPrank();

        (uint256 deposited) = pool.getAssetsOf(linearRatePool, user);
        console.log("total assets deposited ->", deposited);

        assertEq(sharesBefore, sharesAfter);
        assertEq(newTotalBorrowShares, totalBorrowShares);
        assertGt(newTotalBorrowAssets, totalBorrowAssets);
    }

In Superpool.t.sol

function testInterestEarnedOnTheUnderlingPool() public {
        // 1. Setup a basic pool with an asset1
        // 2. Add it to the superpool
        // 3. Deposit assets into the pool
        // 4. Borrow from an alternate account
        // 5. accrueInterest
        // 6. Attempt to withdraw all of the liquidity, and see the running out of the pool
        vm.startPrank(poolOwner);
        superPool.addPool(linearRatePool, 50 ether);
        superPool.addPool(fixedRatePool, 50 ether);
        vm.stopPrank();

        vm.startPrank(user);
        asset1.mint(user, 50 ether);
        asset1.approve(address(superPool), 50 ether);

        vm.expectRevert();
        superPool.deposit(0, user);

        superPool.deposit(50 ether, user);
        vm.stopPrank();

        vm.startPrank(Pool(pool).positionManager());
        Pool(pool).borrow(linearRatePool, user, 35 ether);
        vm.stopPrank();

        vm.warp(block.timestamp + 365 days);
        vm.roll(block.number + 5_000_000);
        pool.accrue(linearRatePool);

        vm.startPrank(user2);
        asset1.mint(user2, 421 ether);
        asset1.approve(address(superPool), 421 ether);

        (uint256 sharesMinted) = superPool.deposit(421 ether, user2);
        console.log("Shares Minted ->", sharesMinted);
        (uint256 sharesBurned) = superPool.withdraw(421 ether, user2, user2);
        console.log("Shares Burned ->", sharesBurned);
        vm.stopPrank();

        vm.startPrank(Pool(pool).positionManager());
        uint256 borrowsOwed = pool.getBorrowsOf(linearRatePool, user);

        asset1.mint(Pool(pool).positionManager(), borrowsOwed);
        asset1.approve(address(pool), borrowsOwed);
        Pool(pool).repay(linearRatePool, user, borrowsOwed);
        vm.stopPrank();

        superPool.accrue();

        vm.startPrank(user);
        vm.expectRevert(); // Not enough liquidity
        superPool.withdraw(40 ether, user, user);
        vm.stopPrank();

        vm.startPrank(poolOwner);
        vm.expectRevert(); // Cant remove a pool with liquidity in it
        superPool.removePool(linearRatePool, false);
        vm.stopPrank();
    }

Mitigation

Changing Math.Rounding.Up to Mat.Rounding.Down in Pool.sol:350 solves the issue.

[S3v3ru5]

I consider this issue to be invalid:

The issue describes two impacts

1. Loss of funds to users from rounding-up the shares in Pool.withdraw function
2. DoS in SuperPool when it calls Pool.withdraw

For (1), it is intended to round-up. The rounding-direction should always favour the protocol, so it should round-up. Also, the loss of funds to users is very minimal (1 or 2 wei). The loss is the value of extra shares because of rounding up and that value is always less than totalSupply / totalShares.

If the withdraw function is rounded-down as suggested in mitigation, then protocol is burning less shares, its a loss for the protocol.

For (2), the shown DoS is that superPool cannot remove a pool with liquidity in it. The PoC test sets the forceRemove parameter of SuperPool.removePool to false. If the forceRemove is set to false then the Pool.withdraw function is not even called which was the shown root cause.

Also setting forceRemove to True, the SuperPool will withdraw the full assets and can close the pool.

removePool:

    function removePool(uint256 poolId, bool forceRemove) external onlyOwner {
        if (poolCapFor[poolId] == 0) return; // no op if pool is not in queue
        uint256 assetsInPool = POOL.getAssetsOf(poolId, address(this));
        if (forceRemove && assetsInPool > 0) POOL.withdraw(poolId, assetsInPool, address(this), address(this));
        _removePool(poolId);
        poolCapFor[poolId] = 0;
        emit PoolCapSet(poolId, 0);
    }

If forceRemove is set to false then withdraw is never called.

        uint256 assetsInPool = POOL.getAssetsOf(poolId, address(this));
        if (forceRemove && assetsInPool > 0) POOL.withdraw(poolId, assetsInPool, address(this), address(this));

Because rounding-up the shares is intended in withdraw function, the loss is negligible, and there's no DoS, I consider this issue to be invalid.

[AtanasDimulski]

Escalate, Per the above comment

[sherlock-admin3]

Escalate, Per the above comment

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.

[Kalogerone]

Additional Input:

is also not compliant with ERC 4626.

Making this statement without any source of truth is questionable. OpenZeppelin's ERC4626 also rounds UP when withdrawing and their implementation is EIP 4626 compliant.

[cvetanovv]

I agree with the escalation.

The protocol rounds up correctly. We can see that OpenZeppelin's ERC4626 also does it this way.

Also, @S3v3ru5 has correctly identified that the PoC test is incorrect, and we have no issue.

Planning to accept the escalation and invalidate the issue.

[Mihir018]

@cvetanovv I disagree with it, as you can see the pool will be in withdraw queue in SuperPool but withdraw function from SuperPool will fail because the withdraw function will not let the liquidity to be withdrawn in full from underlying pool due to rounding

[ruvaag]

Agree with the escalation, this should be invalid.

It seems to be a result of iliquidity and not rounding. Additionally the Pool.withdraw rounding direction is intended to be in favor of the protocol

[cvetanovv]

The sponsor confirms that this is an intended design, and we have no issue here. For this, my decision remains.

My decision to accept the escalation and invalidate the issue remains.

[Mihir018]

But it can create a situation of DOS if in superPool contract it assumes that underlying pool in withdraw queuw has that much amount of liquidity which can be withdrawn but that transaction reverts due to rounding calculation, that is issue right ?

[cvetanovv]

@Mihir018 We don't have a DoS situation in SuperPool. See this comment: https://github.com/sherlock-audit/2024-08-sentiment-v2-judging/issues/601#issuecomment-2352078735

My decision to accept the escalation and invalidate the issue remains.

[Evert0x]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• AtanasDimulski: accepted