xiaoming90 - `block.timestamp` should be used instead of `block.number`

[github-actions[bot]]

xiaoming90

medium

block.timestamp should be used instead of block.number

Summary

The reward distribution should be a variable of time instead of a block number, as the reward distribution is the ratio of the timeframe to "time since the last harvest" per the source code's comment. Thus, block.timestamp should be used here instead of block.number to ensure the amount of rewards streamed or distributed continuously over a period of time

Vulnerability Detail

https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/utils/RewardRouter.sol#L38

File: RewardRouter.sol
34:     /// @dev Distributes grant rewards and triggers reward collector to claim and donate
35:     function distributeRewards(address vault) external returns (uint256) {
36:         // If vault is set to receive rewards from grants, send amount to reward collector to donate
37:         if (rewards[vault].rewardAmount > 0) {
38:             // Calculates ratio of timeframe to time since last harvest
39:             // Uses this ratio to determine partial reward amount or extra reward amount
40:             uint256 blocksSinceLastReward = block.number - rewards[vault].lastRewardBlock;
41:             uint256 maxReward = rewards[vault].rewardAmount - rewards[vault].rewardPaid;
42:             uint256 currentReward = rewards[vault].rewardAmount * blocksSinceLastReward / rewards[vault].rewardTimeframe;
43:             uint256 amountToSend = currentReward > maxReward ? maxReward : currentReward;

Currently, the rewards are distributed based on the number of blocks that have been passed since the block of the last reward distribution/harvest. As a result, the block.number is used to fetch the current L2 block number.

However, per the comment in Line 38 above:

// Calculates ratio of timeframe to time since last harvest

As such, the reward distribution should be a variable of time instead of a block number, as the reward distribution is the ratio of the timeframe to "time since the last harvest" per the source code's comment. Thus, block.timestamp should be used here instead to ensure the amount of rewards streamed or distributed continuously over a period of time.

Impact

Using block.number could potentially lead to a number of pitfalls or impacts in this case:

1. A block is minted in Optimism every 2 seconds, while a block is minted in Ethereum every 12 seconds. Thus, the admin might erroneously apply the wrong block minting period (12 seconds instead of 2 seconds) when computing the reward timeframe, leading to a shorter timeframe and faster reward distribution.
2. When the sequencer is stopped for a certain reason, the block minting is also paused, and the block number will stop increasing. However, while the sequencer is stopped, the reward router will fail to take into consideration the amount of time that has already passed when computing the number of rewards to be distributed when the sequencer resumes later. Thus, it breaks the requirement of distributing the rewards based on the ratio of timeframe to time since the last harvest, as mentioned earlier.

Code Snippet

https://github.com/sherlock-audit/2024-04-alchemix/blob/main/v2-foundry/src/utils/RewardRouter.sol#L38

Tool used

Manual Review

Recommendation

Consider using block.timestamp instead of block.number while computing the number of rewards to distribute.

[Hesnicewithit]

This is a good improvement for when we deploy on new L2s. Will implement this change

[gstoyanovbg]

Escalate

I agree that this is a good improvement but don't think that is a security issue. At this moment it sounds to me like Low severity and this is why i escalate it for further discussion.

[sherlock-admin3]

Escalate

I agree that this is a good improvement but don't think that is a security issue. At this moment it sounds to me like Low severity and this is why i escalate it for further discussion.

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.

[Hash01011122]

Agreed, this is borderline low/medium severity issue, the reason I choose medium for this is because

While the sequencer is stopped, the reward router will fail to take into consideration the amount of time that has already passed when computing the number of rewards to be distributed when the sequencer resumes later. Thus, it breaks the requirement of distributing the rewards based on the ratio of timeframe to time since the last harvest.

It indirectly affects the core functionality of protocol Would you like to add anything @xiaoming9090??

[WangSecurity]

Fair argument from @Hash01011122 regarding sequencer problem, but as stated in the README, the sequencer is assumed to not misbehave or go offline. Moreover, I see this report as a design decision, rather than the security issue.

Planning to accept the escalation and invalidate the issue.

[Slavchew]

Isn't the main problem here as stated in the report that the block durations in L2 vs L1 are shorter, which leads to faster rewards?

A block is minted in Optimism every 2 seconds, while a block is minted in Ethereum every 12 seconds. Thus, the admin might erroneously apply the wrong block minting period (12 seconds instead of 2 seconds) when computing the reward timeframe, leading to a shorter timeframe and faster reward distribution.

[WangSecurity]

After re-reading the report, I agree. The block on L2s is faster, hence, users on L2 will receive rewards faster than the users on Ethereum.

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

[Hash01011122]

Agree with what @WangSecurity pointed out here. Escalation can be rejected.

[gstoyanovbg]

A block is minted in Optimism every 2 seconds, while a block is minted in Ethereum every 12 seconds. Thus, the admin might erroneously apply the wrong block minting period (12 seconds instead of 2 seconds) when computing the reward timeframe, leading to a shorter timeframe and faster reward distribution.

The timeFrame is something that the admin sets per vault and it is responsibility of the admin to set the correct value for the respective chain. In the README file is stated that the admin is Trusted so admin mistakes should not be considered valid issues. This is a Sherlock's rule that supports the claim:

Admin Input/call validation: Protocol admin is considered to be trusted in most cases, hence issues where 1.Admin incorrectly enters an input parameter. Example: Make sure interestPerMin > 1 ether as it is an important parameter. This is not a valid issue. 2.Admin could have an incorrect call order. Example: If an Admin forgets to setWithdrawAddress() before calling withdrawAll() This is not a valid issue. 3.An admin action can break certain assumptions about the functioning of the code. Example: Pausing a collateral causes some users to be unfairly liquidated or any other action causing loss of funds. This is not considered a valid issue.

When the sequencer is stopped for a certain reason, the block minting is also paused, and the block number will stop increasing. However, while the sequencer is stopped, the reward router will fail to take into consideration the amount of time that has already passed when computing the number of rewards to be distributed when the sequencer resumes later. Thus, it breaks the requirement of distributing the rewards based on the ratio of timeframe to time since the last harvest, as mentioned earlier.

This is not a valid point too because according the README:

It is acceptable to assume the sequencer won't misbehave or go offline.

[WangSecurity]

The sequncer concerns are indeed not a valid reason to make it valid, cause based on README we assume they won't misbehave and/or go offline. Admin sets the timeframe and since they're trusted, the timeframe will be appropriate. But, I believe the trusted admin rule cannot be applied here, cause the blocks are still mined/validated at a different speed and users on different chains will receive more and faster rewards than on others. Hence, planning to reject the escalation and leave the issue as it is.

[gstoyanovbg]

@WangSecurity I don't quire understand the last comment.

But, I believe the trusted admin rule cannot be applied here, cause the blocks are still mined/validated at a different speed and users on different chains will receive more and faster rewards than on others

This would be true only if the admin set a wrong time frame but in the previous sentence you stated the admin will set appropriate time frame. Do i miss something ?

[WangSecurity]

I meant that the admin can calculate the appropriate timeFrame variable so the rewards are the same on all the chains. But, still every block is mined/validated with the different speed. For example, on Ethereum it's not always 12 secs and on Optimism it's not always 2 secs, it can be 14 and 1, or 2.5 and 11. What I mean is that it's impossible to guess at what time every next block will be mined/validated, hence, the rewards on different chains will indeed be distributed at a different speed, regardless of the owner setting correct or incorrect timeframe. And I believe it cannot happen if we use timestamp insted of number, but if I'm mistaken here, please correct me.

[gstoyanovbg]

@WangSecurity I agree with what you said. We can summarize that this would only be a problem if the given chain does not produce blocks regularly. But in my opinion, even if this happens, it is extremely rare. Here you can see the latest blocks on OP. I've reviewed the last few days and didn't notice any irregular blocks; they were always every 2 seconds. This is an example of a similar report from a previous contest (it's not source of truth, I'm just showing that this issue has been discussed and arguments that it happens frequently have not been found).

I also want to note that although your analysis is correct, it is not presented in this report. The only thing claimed in the original report is that the administrator may make a mistake by choosing a wrong value. I think we agreed that this is not an argument for validating this report because the administrator is trusted. Also, no evidence supporting the claim that there is irregular block production on a specific chain is presented. Therefore, this is speculation.

One more thing independent from the above - i am not sure what is the impact and which category from the criteria for Medium severity issues describe this report ?

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss. Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

[WangSecurity]

Hm, that's actually very insightful. In that case the different speed of receiving rewards is possible, but it has extremely low probability and the impact is that users may receive rewards later a second later or even less, correct?

I'll also give some time to the defending side to provide counter-arguments. Thank you very much!

[Slavchew]

I agree that this report relies heavily on admin errors but all the other reports show how in all the different chains block mining is different and each chain has its own block number, and as @WangSecurity stated it would get many more awards in a faster timeframe instead if you use block.timestamp which is the same for all chains.

An example of a BASE chain that blocks are with very different numbers compared to Mainnet.

[WangSecurity]

Can any of you please forward me to the function where the admin sets rewards and its parameters?

[gstoyanovbg]

@Slavchew The screenshots from your comment confirm my statement that irregular block is something extremely rare because the block producing is consistent there - 2s for OP and 12s for Ethereum

@WangSecurity https://github.com/sherlock-audit/2024-04-alchemix/blob/32e4902f77b05ea856cf52617c55c3450507281c/v2-foundry/src/utils/RewardRouter.sol#L66-L73

https://github.com/sherlock-audit/2024-04-alchemix/blob/32e4902f77b05ea856cf52617c55c3450507281c/v2-foundry/src/utils/RewardRouter.sol#L86-L88

[WangSecurity]

I think I agree with @gstoyanovbg here. Firstly, the blocks are indeed are very consistent and the probability of them being mined/validated at a different speed (for example 3 secs insted of 2 on OP and 11 secs insted of 12 on ETH). Secondly, the admin can set the Reward struct parameters in that way, so rewards on all chains are distributed in the same amounts for the same time (supposedly we want to distribute 3 ETH over 1 week on Ethereum and Optimism and the admin can indeed set the parameters so the rewards are distributed in the same way). Correct?

Hence, the only way the same rewards will be distributed with different speed can only happen if the admin makes a mistake or the blocks start to mine/validate at the different speed. Correct?

[gstoyanovbg]

I think I agree with @gstoyanovbg here. Firstly, the blocks are indeed are very consistent and the probability of them being mined/validated at a different speed (for example 3 secs insted of 2 on OP and 11 secs insted of 12 on ETH). Secondly, the admin can set the Reward struct parameters in that way, so rewards on all chains are distributed in the same amounts for the same time (supposedly we want to distribute 3 ETH over 1 week on Ethereum and Optimism and the admin can indeed set the parameters so the rewards are distributed in the same way). Correct?

Hence, the only way the same rewards will be distributed with different speed can only happen if the admin makes a mistake or the blocks start to mine/validate at the different speed. Correct?

Correct.

[Slavchew]

I think I agree with @gstoyanovbg here. Firstly, the blocks are indeed are very consistent and the probability of them being mined/validated at a different speed (for example 3 secs insted of 2 on OP and 11 secs insted of 12 on ETH). Secondly, the admin can set the Reward struct parameters in that way, so rewards on all chains are distributed in the same amounts for the same time (supposedly we want to distribute 3 ETH over 1 week on Ethereum and Optimism and the admin can indeed set the parameters so the rewards are distributed in the same way). Correct?

Hence, the only way the same rewards will be distributed with different speed can only happen if the admin makes a mistake or the blocks start to mine/validate at the different speed. Correct?

@WangSecurity I agree with you that it's almost impossible for the block mint duration to differ, but you went in a different direction and that is not the problem, I will try to explain how exactly because of the block.number of L2 chains the rewards will be much quickly.

Here is an example that no matter what values are set in the vault Reward struct, in all L2 chains the rewards will be faster.

Consider this scenario

• 2 chains - BASE and Mainnet (Base 2 sec block, Mainnet 12 sec block)
• the same Vault Reward struct is added for both chains
• Vault with:
  • address rewardCollectorAddress - the address
  • uint256 rewardAmount - 2e18 (2 tokens for simplicity)
  • uint256 rewardPaid - 0
  • uint256 rewardTimeframe - 2000 (distributing rewardAmount for 2000 blocks)
  • uint256 lastRewardBlock - block.number (because it is added now and no rewards are distributed)

function distributeRewards(address vault) external returns (uint256) {
        // If vault is set to receive rewards from grants, send amount to reward collector to donate
        if (rewards[vault].rewardAmount > 0) {
            // Calculates ratio of timeframe to time since last harvest
            // Uses this ratio to determine partial reward amount or extra reward amount
            uint256 blocksSinceLastReward = block.number - rewards[vault].lastRewardBlock;
            uint256 maxReward = rewards[vault].rewardAmount - rewards[vault].rewardPaid;
            uint256 currentReward = rewards[vault].rewardAmount * blocksSinceLastReward / rewards[vault].rewardTimeframe;
            uint256 amountToSend = currentReward > maxReward ? maxReward : currentReward;

For all 2000 blocks to pass on Mainnet it will take 2000*12 = 24000 sec = 400 min = 6:40 hours

In contrast, on Base where it will take 2000*2 = 4000 sec = 66.6 min = 1:06 hours For these 1:06 hours, only 333 blocks will minted on Mainnet.

If block.timestamp is used, all calculations will be the same no matter which chain.

I'm tagging the owner of this @xiaoming9090 to share his thoughts on it as well.

[WangSecurity]

That's correct, but still the owner can set the Reward parameters in the way that the same amount of rewards will be distributed at the same time, i.e. 2e18 will be distributed in 6:40 hours or 1:06 hours on both chains, correct?

[Slavchew]

That's correct, but still the owner can set the Reward parameters in the way that the same amount of rewards will be distributed at the same time, i.e. 2e18 will be distributed in 6:40 hours or 1:06 hours on both chains, correct?

Exactly, this is also true that the owner can calculate how much he needs to pass to any chain to match the Mainnet distribution, but why should he even do that when he can, as the sponsor said, just change it to block.timestamp and everything will be fine, that way they skip all those block calculations.

Because if they want BASE to match the Mainnet distribution for 6:40 hours, they need to calculate how many blocks will be mined in that time on BASE and pass it on, which shows that they will be very related to the hours, and not the blocks, which again shows that with the solution of the problem, everything will be avoided.

Besides that I want to share the biggest issue here which is on Arbitrum you can check #9 it summarizes it perfectly.

[WangSecurity]

Hm, thank you for bringing it up, but if we look at the time of Arbitrum blocks mined/validated, it looks to be consistently 4 blocks/sec. Then, it's still possible to calculate and set the Reward parameters in a way so rewards on every chain are distributed in the same time, correct?

I understand that block.timestamp would ease the proccess and eliminate all the accounting. The questions here is that the admin is TRUSTED, hence, we have to assume they do everything correctly and won't make such mistakes. Therefore, I'm trying to understand if it's completely in their control. (if the image doesn't load, here is a link to arbitrum blocks, correct me if I'm looking at something incorrect).

[Slavchew]

I don't think we can rely on pervious 20-30 blocks even 1000, but to the Arbitrum docs.

[WangSecurity]

Fair enough, but before making the last decision, will give some time to @gstoyanovbg to provide arguments.

[WangSecurity]

To clarify this part from the docs:

ArbOS and the sequencer are responsible for delineating when one Arbitrum block ends and the next one begins. However, block creation depends entirely on chain usage, meaning that blocks are only produced when there are transactions to sequence. In active chains, one can expect to see Arbitrum blocks produced at a relatively steady rate. In more quiet chains, block production might be sporadic depending on the rate at which transactions are received.

It's relevant to Arbitrum as a whole or only on other chains that are built on ArbOS? Just a bit confused regarding "In active chains" and "in more quite chains". If we take Abitrum solely, it still means there are not many txns on Arbitrum it will produce blocks at a lower pace, correct?

[gzeoneth]

Hi Arbitrum dev here, Arbitrum chains return L1 block number with block.number, L2(or L3) block times are irrelevant.

[WangSecurity]

So if we call block.number on Arbitrum, we get Ethereum's block number? @gzeoneth

[gzeoneth]

So if we call block.number on Arbitrum, we get Ethereum's block number? @gzeoneth

Correct

[WangSecurity]

It's proven by the dev and by Arbitrum's docs that if we deploy a smart contract on Arbitrum and call block.number on it, we get Ethereum's block number, not Arbitrum's. Hence, the rewards parameters can be the same on Ethereum and Arbitrum and the rewards will be distributed in the same time. Distributing rewards on Optimisim in the same time requires a TRUSTED admin to set parameters respectively. The only constraint left here is that it will take different amount of time to mine/validate each block, probability of which is extremely low and the impact is low as well (the rewards will be distributed a couple of seconds earlier/later).

I agree that changing block.number to block.timestamp will be an improvement and will eliminate the accounting to distribute rewards in the same time. But, it's only a design improvement, hence, the report should be low/info.

Planning to accept the escalation and invalida an issue.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/alchemix-finance/v2-foundry/commit/f0e7530cde2c006fd13c7c34b695113679a9655b

[Evert0x]

Result: Invalid Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• gstoyanovbg: rejected

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.