`RepricingToken::checkpointReserves()` - L168: Should use `<=` here, NOT `<`, otherwise it allows for checkpointing pending reserves BEFORE the `reservesVestingDuration` has completely passed.

[hats-bug-reporter[bot]]

Github username: -- Twitter username: 0xSCSamurai Submission hash (on-chain): 0xf073ac2cc477710c609d4a5ae315ef138eaafe5c92123cd4560e27984fc90ab7 Severity: medium

Description: Description\

RepricingToken::checkpointReserves() - L168: Should use <= here, NOT <, otherwise it allows for checkpointing pending reserves BEFORE the reservesVestingDuration has completely passed.

https://github.com/hats-finance/Origami-0x998f1b716a5022be026ca6b919c0ddf45ca31abd/blob/185a93e25071b6a110ca190e94a6a826e982b2d6/apps/protocol/contracts/common/RepricingToken.sol#L165-L170

THE LOGIC: In the below code snippet from the affected function, it's supposed to check and ensure that checkpointing can only be performed AFTER the reservesVestingDuration as passed, this means that we should only do checkpointing once the following condition is true: block.timestamp - lastVestingCheckpoint > reservesVestingDuration, which negates to <=. Therefore we need to use <= and NOT <, otherwise checkpointing can start happening during same block as the final block of reservesVestingDuration period, which is obviously not intended protocol logic.

This line incorrectly allows for checkpointing before reservesVestingDuration period has passed: if (block.timestamp - lastVestingCheckpoint < reservesVestingDuration) revert CannotCheckpointReserves(block.timestamp - lastVestingCheckpoint, reservesVestingDuration);

Affected function:

    /// @notice Checkpoint any pending reserves as long as the `reservesVestingDuration` period has completely passed. 
    /// @dev No economic benefit, but may be useful for book keeping purposes.
    function checkpointReserves() external override {
        if (block.timestamp - lastVestingCheckpoint < reservesVestingDuration) revert CannotCheckpointReserves(block.timestamp - lastVestingCheckpoint, reservesVestingDuration); 
        _checkpointAndAddReserves(0);
    }

Attack Scenario\ n/a

Attachments

1. Proof of Concept (PoC) File

THE LOGIC: In the below code snippet from the affected function, it's supposed to check and ensure that checkpointing can only be performed AFTER the reservesVestingDuration as passed, this means that we should only do checkpointing once the following condition is true: block.timestamp - lastVestingCheckpoint > reservesVestingDuration, which negates to <=. Therefore we need to use <= and NOT <, otherwise checkpointing can start happening during same block as the final block of reservesVestingDuration period, which is obviously not intended protocol logic.

2. Revised Code File (Optional)

Recommended bugfix:

    /// @notice Checkpoint any pending reserves as long as the `reservesVestingDuration` period has completely passed. 
    /// @dev No economic benefit, but may be useful for book keeping purposes.
    function checkpointReserves() external override {
-       if (block.timestamp - lastVestingCheckpoint < reservesVestingDuration) revert CannotCheckpointReserves(block.timestamp - lastVestingCheckpoint, reservesVestingDuration); 
+       if (block.timestamp - lastVestingCheckpoint <= reservesVestingDuration) revert CannotCheckpointReserves(block.timestamp - lastVestingCheckpoint, reservesVestingDuration);
        _checkpointAndAddReserves(0);
    }

[frontier159]

Thanks for the report.

I disagree with this finding however, since the accrued reserves is the same when block.timestamp - lastVestingCheckpoint == reservesVestingDuration, or if it is one second later.

There's a test showing this here: https://github.com/TempleDAO/origami-public/blob/185a93e25071b6a110ca190e94a6a826e982b2d6/apps/protocol/test/hardhat/common/repricing-token.ts#L456

Please provide a PoC if you think otherwise.

[dappconsulting]

Not sure why everyone says the same thing always, 1 second later? My logic is that it's one block later, not necessarily 1 second later, but 1 block later. In other words, my argument is that according to protocol logic checkpointing pending reserves should be processed/executed in a block AFTER the last block of reservesVestingDuration.

Because in the same block there is always the good chance that the checkpointing tx will be included in the transaction queue for the block BEFORE the reservesVestingDuration transaction, which is something we don't want, as it violates the logic of processing it only AFTER the reservesVestingDuration.

I could argue that even if the results are exactly the same, it doesn't matter, because we need to keep in line with the required logic.

If you implemented my bugfix, how would it affect your protocol? If nothing negative, if protocol still functions correctly, then implementing my suggested bugfix is something to be seriously considered, because as far as I'm concerned, my logic is correct, despite the fact that per your argument the results might be the same. This will not always be the case in some scenarios/other protocols and to ensure we 100% eliminate any issues, implementing my bugfix would be the wise thing to do.

Additionally, this isn't the same moment as what my bug report is pointing out: await mineForwardSeconds(reservesVestingDuration-1); // Right on the vesting time The above from your test is pointing to a moment or time, maybe 1 second before the last moment or last second of the vesting duration. I'm pointing out that your implemented logic is allowing for checkpointing to be done AT the same time as the final time or block or second of the reservesVestingDuration period. Your test seems to test for 1 second before this, therefore not covering what I pointed out?

The bottom line is that my argument and logic should be intact. If your protocol logic ALLOWS for checkpointing to be executed BEFORE vesting duration period has 100% ended, then my bug report is invalid, otherwise it's logically valid.

I will try do the PoC. I see your argument, but we need to respect the mathematical logic always, no matter if it makes any difference or not. It should not be a valid argument to leave it as it is if the mathematical logic is not correct.

Anyway, I will try do the PoC for you.

[dappconsulting]

I'm not having any luck getting my test environment working with your tests. I'm pretty efficient at getting this working generally but no cigar with your codebase. Maybe I did something wrong during setup. Not going to spend any more time on this. Has been HOURS.

The test you pointed out to me, as mentioned previously, it seems it doesnt cover the case that my report is pointing out. Can you please check for me.

thanx.