sherlock-audit / 2024-08-sentiment-v2-judging

3 stars 2 forks source link

ZeroTrust - Accrued interest is calculated incorrectly due to a continuous griefing attack. #562

Closed sherlock-admin2 closed 2 months ago

sherlock-admin2 commented 2 months ago

ZeroTrust

Medium

Accrued interest is calculated incorrectly due to a continuous griefing attack.

Summary

In the Arbitrum L2 environment, low transaction fees enable malicious users to exploit Pool::accrue() function, resulting in Accrued interest incorrect.

Vulnerability Detail

@>>    function accrue(uint256 id) external {
        PoolData storage pool = poolDataFor[id];
        accrue(pool, id);
    }

We can see that accrue can be called by any external user. The accrue() function calls accrue(pool, id), which internally calls simulateAccrue(). Ultimately, interest is calculated through the RateMode::getInterestAccrued() function.

        function accrue(PoolData storage pool, uint256 id) internal {
@>        (uint256 interestAccrued, uint256 feeShares) = simulateAccrue(pool);

        if (feeShares != 0) _mint(feeRecipient, id, feeShares);

        // update pool state
        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);
    }
    function simulateAccrue(PoolData storage pool) internal view returns (uint256, uint256) {
@>>        uint256 interestAccrued = IRateModel(pool.rateModel).getInterestAccrued(
            pool.lastUpdated, pool.totalBorrowAssets, pool.totalDepositAssets
        );
        //skip ......
        return (interestAccrued, feeShares);
    }

Let’s take the FixedRateModel as an example to examine the getInterestAccrued function. Other RateModel implementations follow a similar pattern.

   function getInterestAccrued(uint256 lastUpdated, uint256 totalBorrows, uint256) external view returns (uint256) {
        // [ROUND] rateFactor is rounded up, in favor of the protocol
        // rateFactor = time delta * apr / secondsPerYear
@>>        uint256 rateFactor = ((block.timestamp - lastUpdated)).mulDiv(RATE, SECONDS_PER_YEAR, Math.Rounding.Up);

        // [ROUND] interest accrued is rounded up, in favor of the protocol
        // interestAccrued = borrows * rateFactor
@>>        return totalBorrows.mulDiv(rateFactor, 1e18, Math.Rounding.Up);
    }    

Since the internal calculations in getInterestAccrued use Math.Rounding.Up for approximations, this results in accrued interest being slightly higher than the actual interest. This issue becomes more pronounced for tokens like USDT and USDC, which have precision of 6 or lower. When subjected to continuous accrue() calls in a griefing attack, the problem can escalate, leading to an overestimation of interest.

POC

Assume the token is USDC, with a precision of 6, and the fixed annual interest rate is 5% (RATE = 5e16). The borrowed amount is 1,000,000.

function testTimeIncreasesDebtIncorrect() public {
        uint96 assets = 5_000_000;
        testBorrowWorksAsIntended(assets);
        (,,,,,,, uint256 totalBorrowAssets, uint256 totalBorrowShares,,) = pool.poolDataFor(linearRatePool);
        console2.log("totalBorrowAssets is: ", totalBorrowAssets);

        //after 1 second
        vm.warp(block.timestamp+1);
        vm.roll(block.number + 1);

        pool.accrue(linearRatePool);

        (,,,,,,, uint256 newTotalBorrowAssets, uint256 newTotalBorrowShares,,) = pool.poolDataFor(linearRatePool);
        console2.log("newTotalBorrowAssets is: ", newTotalBorrowAssets);
        assertEq(newTotalBorrowShares, totalBorrowShares);
        assertGt(newTotalBorrowAssets, totalBorrowAssets);
    }

Add the above function to ./test/core/Pool.t.sol and modify two values in the BaseTest.t.sol file.

-   asset1 = new MockERC20("Asset1", "ASSET1", 6);
+   asset1 = new MockERC20("Asset1", "ASSET1", 18);

-   address fixedRateModel = address(new FixedRateModel(1e18));
+   address fixedRateModel = address(new FixedRateModel(5e16));

Then run the command: forge test --mt testTimeIncreasesDebtIncorrect -vv This will produce the test result output.

[PASS] testTimeIncreasesDebtIncorrect() (gas: 357566)
Logs:
  totalBorrowAssets is:  1000000
  newTotalBorrowAssets is:  1000001

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.47ms (599.29µs CPU time)

Thus, the actual generated annual interest rate is: 1*31557600/1000000 = 3155.76%, which is far greater than 5%.

Impact

The borrower ends up paying significantly more interest, leading to financial losses.

Code Snippet

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/Pool.sol#L375

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/Pool.sol#L401

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/Pool.sol#L380

https://github.com/sherlock-audit/2024-08-sentiment-v2/blob/0b472f4bffdb2c7432a5d21f1636139cc01561a5/protocol-v2/src/irm/FixedRateModel.sol#L30

Tool used

Manual Review

Recommendation

Add access control to the accrue function.​

z3s commented 2 months ago

PoC has wrong impact data

ZeroTrust01 commented 2 months ago

Escalate The POC showed the correct data. For a principal of 1,000,000, the interest generated in 1 second is 1, so the interest for one year is 31,557,600.

sherlock-admin3 commented 2 months ago

Escalate The POC showed the correct data. For a principal of 1,000,000, the interest generated in 1 second is 1, so the interest for one year is 31,557,600.

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.

ruvaag commented 1 month ago

This should be invalid, from what i understand if an "attacker" continuously calls 'accrue()' all that will happen is them losing a lot of money at the cost of interest being compounded at a rate of block production.

ZeroTrust01 commented 1 month ago

This is exactly where I pointed out the problem: an ‘attacker’ can cause the pool’s interest to be far greater than 5%. The attacker doesn’t profit, but can cause losses for the borrower.

cvetanovv commented 1 month ago

Although PoC works, there are many unknowns.

We'll take Polygon as an example, where gas is cheap: https://polygonscan.com/gastracker

Currently, one such transaction will cost around 0.25$.

A block in Polygon is 2 seconds, so he needs to call the function 15778800 times.

15,778,800 * 0.26 = $4,102,488

So, someone has to spend over 4 million dollars to inflate the interest rate on one of the cheapest networks.

This issue is no more than Low/Information severity.

Planning to reject the escalation and leave the issue as is.

Evert0x commented 1 month ago

Result: Invalid Unique

sherlock-admin4 commented 1 month ago

Escalations have been resolved successfully!

Escalation status: