Rate limit will be updated every transaction instead of every block

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-03-ondo-finance/blob/main/contracts/InstantMintTimeBasedRateLimiter.sol#L93-L109

Vulnerability details

Impact

In the _checkAndUpdateInstantMintLimit function

  function _checkAndUpdateInstantMintLimit(uint256 amount) internal {
    require(amount > 0, "RateLimit: mint amount can't be zero");

    if (
      block.timestamp >= lastResetInstantMintTime + resetInstantMintDuration
    ) {
      // time has passed, reset
      currentInstantMintAmount = 0;
      lastResetInstantMintTime = block.timestamp;
    }
    require(
      amount <= instantMintLimit - currentInstantMintAmount,
      "RateLimit: Mint exceeds rate limit"
    );

    currentInstantMintAmount += amount;
  }

which is used in the _mint function, there is an if statement which checks if the current block.timestamp is more than or equal to lastResetInstantMintTime + resetInstantMintDuration. Given the current setup

resetInstantMintDuration = _instantMintResetDuration; // can be zero for per-block limit

the rate limit should be updated every block if the resetInstantMintDuration is 0, but this is not the case.

Proof of Concept

With the current implementation:

block.timestamp >= lastResetInstantMintTime + resetInstantMintDuration

and resetInstantMintDuration being set as 0, the rate limit will be updated with every transaction.

Tools Used

Manual Review

Recommended Mitigation Steps

Implement the following changes:

function _checkAndUpdateInstantMintLimit(uint256 amount) internal {
    require(amount > 0, "RateLimit: mint amount can't be zero");

    if (
      block.number > lastResetInstantMintBlock
    ) {
      // time has passed, reset
      lastResetInstantMintBlock = block.number;
    }
    require(
      amount <= instantMintLimit - currentInstantMintAmount,
      "RateLimit: Mint exceeds rate limit"
    );

    currentInstantMintAmount += amount;
  }

Assessed type

Timing

[c4-pre-sort]

0xRobocop marked the issue as insufficient quality report

[3docSec]

if (
      block.timestamp >= lastResetInstantMintTime + resetInstantMintDuration
    ) 

when resetInstantMintDuration will always be true on every new block, so it meets the expectation of resetting the ramp every block

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid

[notbozho]

Hey, @3docSec

Given the fact that resetInstantMintDuration is not a bool, but a uint265 - it can be set to zero(as planned in the constructor)

resetInstantMintDuration = _instantMintResetDuration; // can be zero for per-block limit

but your comment is not true, because resetInstantMintDuration being 0 will result in

if (
      block.timestamp >= lastResetInstantMintTime + resetInstantMintDuration
    )

to always be true and in currentInstantMintAmount being set to 0 every transaction.

I am aware that there is a setInstantMintLimit function, but from deployment to the moment the admin has realized and changed the resetInstantMintDuration', thecurrentInstantMintAmount` will always be 0 and the

require(
      amount <= instantMintLimit - currentInstantMintAmount,
      "RateLimit: Mint exceeds rate limit"
    );

    currentInstantMintAmount += amount;

will practically work as

require(
      amount <= instantMintLimit - 0,
      "RateLimit: Mint exceeds rate limit"
    );

which means every transaction will be able to mint the instantMintLimit without being reduced as planned for unrestricted amount of time.

Also setting the resetInstantMintDuration to the start of every block would be difficult as the block duration varies most of the times with 2-3 seconds due to several factors.

My reccomendation from above stays the same and I am certain it will prevent all of this problems at once.

[3docSec]

I see that we disagree on a purely technical matter. Could you @notbozho provide a runnable PoC to clear this out?

[radeveth]

function test_instant_mint_twice_in_the_same_block() public {
    deal(address(USDC), alice, 20_000_000e6);
    vm.startPrank(alice);
    USDC.approve(address(ousgInstantManager), 20_000_000e6);
    //Everything is set up exactly as for only one mint, except we mint twice in the same transaction.
    //first mint of the instantMintLimit
    ousgInstantManager.mint(10_000_000e6);
    console.log (block.timestamp);
    //second mint of the instantMintLimit in the same block
    ousgInstantManager.mint(10_000_000e6);
    console.log (block.timestamp);

    vm.stopPrank();

    assertEq(USDC.balanceOf(ousgInstantManager.usdcReceiver()), 20_000_000e6);
    assertEq(USDC.balanceOf(ousgInstantManager.feeReceiver()), 0);
    assertEq(rOUSGToken.sharesOf(alice), 0);
    assertEq(rOUSGToken.balanceOf(alice), 0);
  }

You can paste this in mint.t.sol and check if everything is working as expected. The problem comes from the if statement, which contains an off-by-one issue, allowing minting in the same block indefinite times.

Given the fact that block.timestamp would be the same every transaction until the next block comes and resetInstantMintDuration will be set to 0, the check will pass due to >= operators used together. It has to be > to prevent this scenario.

if (
      block.timestamp >= lastResetInstantMintTime + resetInstantMintDuration
    ) {
      // time has passed, reset
      currentInstantMintAmount = 0;
      lastResetInstantMintTime = block.timestamp;
    }

Also there is the exact same problem in _checkAndUpdateInstantRedemptionLimit function.

[3docSec]

Aha you are right. Thanks for providing a PoC, that's indeed an off-by-one error, my bad I did not follow.

I however doubt this is a medium finding, because the code acts inconsistently with what stated in a comment, and the team can very simply set 1 instead of 0 to achieve the block reset. This is sufficient to let the team know about the issue (valid QA finding), but not sufficient to demonstrate a Medium impact.

[c4-judge]

3docSec removed the grade

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[c4-judge]

3docSec marked the issue as grade-b

[radeveth]

Given the fact we are auditing the current implementation, I am pointing out issues in this particulat commit. I agree that the change is simple and will prevent the issue, but that stands for every finding ever. I believe the idea of the audit itself is to uncover issues which are to be fixed after. Usually protocols start an audit when they think their code is deployment ready, so I once again would like to point out that the issue involves several factors:

1. Funds are involved on minting and redeeming(as I said in the last comment - the exact same issue is present in the _checkAndUpdateInstantRedemptionLimit function.).
2. Unrestricted amount of time before the issue is caught and new value is set.

[cameronclifton]

Thank you for finding. We will be removing this incorrect comment.