29467 - [SC - Low] RewardTimelockexecuteRewardTransaction - L Inco...
RewardTimelock::executeRewardTransaction() - L112: Incorrect comparison operator results in not being able to execute reward tx at the very last second of tx expiration timestamp.
Contract fails to deliver promised returns, but doesn't lose value
Temporary freezing of funds
Description
Brief/Intro
RewardTimelock::executeRewardTransaction() - L112: Incorrect comparison operator results in not being able to execute reward tx at the very last second of tx expiration timestamp.
Disclaimer note:
I'm only including this bug report because it might be of some value to Immunefi team. Initially I got excited because I thought hey I've got a legit bug here, but after deep-diving this and doing multiple PoC test runs I leveled up my insight and understanding of block timestamps, block numbers, and how incorrect use of comparison operators can or cannot affect these... So I dont expect you to validate this report, but who knows maybe you agree with my assessment below.
The bug allows for premature expiration of reward transaction at least 1 second too early, and undermines whitehat experience by delaying reward transfer if the transaction reverts.
Vulnerability Details
SUMMARY:
There are two possible approaches: -- A: something expires AFTER a specific time period, i.e. expires AFTER 60 seconds, i.e. AT 60:00:001 and onwards. OR -- B: something expires AT a specific time, i.e. expires AT exactly 60 seconds onwards, i.e. AT 60:00:000 and onwards.
If the txData.expiration uses approach B, according to intended protocol logic, then this bug report is invalid.
But if txData.expiration uses approach A, according to intended protocol logic, then this bug report is valid.
My assumption is it's approach A, which means the current system's implementation of transaction expiration presents an issue. When setting a 60-second expiration period, as example, transactions are prematurely expiring AT the 60 second mark, instead of at > 60 seconds only. For instance, if a cooldown period ends at precisely 12:00pm and reward transaction expiry is set to 60 seconds later, i.e. 12:01:00:000, the tx should still be valid until that exact moment, and only become expired at 12:01:00:001 onwards. This can result in delays in rewards transfers because the txs will revert.
Impact Details
IMPACT:
Likelihood: High, Impact: Low = Medium severity.
Potential temporary freezing of rewards for the duration of the next cooldown period + expiration, causing frustration for whitehats.
Code snippet in function executeRewardTransaction() where the expiration check is implemented:
Additionally, are we saying that the reward transaction is valid and non-expired during this entire period below(after cooldown), or only up to excluding the last second of this period below?
txData.queueTimestamp + txData.cooldown + txData.expiration == 12:01:00:000. Should the tx be considered expired at:
> 12:01:00:000 ? OR
>= 12:01:00:000 ?
References
Add any relevant links to documentation or code
Proof of Concept
PoC:
Ok, so I've managed to modify the existing tests, and RewardTimelock.sol contract so that it returns the exact difference in seconds between the block.timestamp of the reward tx execution, and the tx expiration timestamp.
So IF this is a bug, then the PoC proves it 100%. If it's not a bug but intended protocol functionality, then the PoC could be something interesting/valuable to check out. Regardless, I had fun taking myself through this process.
The test function I used: https://github.com/immunefi-team/vaults/blob/49c1de26cda19c9e8a4aa311ba3b0dc864f34a25/test/foundry/RewardTimelock.t.sol#L152-L196
My modifications to the executeRewardTransaction() function: https://github.com/immunefi-team/vaults/blob/49c1de26cda19c9e8a4aa311ba3b0dc864f34a25/src/RewardTimelock.sol#L98-L139
Test command: forge test --contracts src/RewardTimelock.sol --mt testTransactionExpiredRevertsExecution -vvv
Note: For all the tests below, I'm emitting event CurrentTransactionBlockNumber which provides the following info for reward transaction: blocknumber, timestamp, expirationTimestamp, timeDifferenceRewardTxBlockvsExpirationBlock(timestamp difference between tx timestamp and expiration timestamp)
Explanation: with your currently implemented logic the reward tx will revert when tx block.timestamp and timestamp of tx expiry overlap, e.g. when timestamp == expirationTimestamp == 950401, but if I make the expiry timestamp 1 second later, then we get the above test result, it does not revert.
With my bugfix it does not revert when timestamp == expirationTimestamp == 950401.
So it's up to Immunefi team's idea of how they want this to work. Do you prefer method A or method B which I described earlier in this report? Either one is OK to use, but consistency is important, as well as clarity. New devs or devs from other protocols, as well as users/whitehats should not be confused or get incorrect ideas of how your protocol logic handles reward tx expiry.
Why is all this so critically important? Because if any of these depended on critical state changes, it matters whether you use > or >=, AND it matters that your dev team remembers why > is used instead of >=. Otherwise invalid state changes could be at risk of happening, although not in this case though.
I will continue hunting for other cases where it might be more serious...
