setWeight Allows Premature Weight Changes Through inadequate minimum time enforcement

[hats-bug-reporter[bot]]

Github username: @ololade97 Twitter username: 97Sabit Submission hash (on-chain): 0xc13f4d2009d58e303db426819e1d3cff28d9d721cb375b252bebab360ce28992 Severity: medium

Description: Description\ Here's the comment in the setWeight function at line 148:

// dev: targetTime must be more than MIN_ADJUSTMENT_TIME in the future.

MIN_ADJUSTMENT_TIME is 7 days between adjustments. setWeight() sets a new target weight for an asset. It requires targetTime >= block.timestamp + MIN_ADJUSTMENT_TIME. Normally this enforces 7 days between weight changes.

However, since targetTime can be equal to the minimum:

A new target weight could be set every 7 days Instead of waiting 7 days after the previous change.

For example:

• On Monday, weight is changed and targetTime is set to the following Monday.
• On next Monday, another adjustment is made, targetTime set to next Monday.
• Weights could change every Monday instead of every other Monday.

This doesn't ensure targetTime is more than MIN_ADJUSTMENT_TIME as specified in the comment.

Attachments https://github.com/catalystdao/catalyst/blob/27b4d0a2bca177aff00def8cd745623bfbf7cb6b/evm/src/CatalystVaultVolatile.sol#L148

1. Proof of Concept (PoC) File

  require(targetTime >= block.timestamp + MIN_ADJUSTMENT_TIME); // dev: targetTime must be more than MIN_ADJUSTMENT_TIME in the future.

2. Revised Code File (Optional)

 require(targetTime > block.timestamp + MIN_ADJUSTMENT_TIME);

[reednaa]

MIN_ADJUSTMENT_TIME refers to the time that an adjustment can take not how often it is set.

[ololade97]

Yes, that's true. But targetTime isn't suppose to be equal to MIN_ADJUSTMENT_TIME. It is suppose to be more than.

targetTime can be equal to MIN_ADJUSTMENT_TIME which is against what the contract envisaged.

[reednaa]

Here is the issue:

1. If the weights could be changed instantly, then the factory owner could drain the vault.
2. This is protected against by saying: A weight change needs to occur over at least MIN_ADJUSTMENT_TIME which is hardcoded to 7 days. https://github.com/hats-finance/Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4/blob/fba322fab023a9206183fb455e9f86facd550d8a/evm/src/CatalystVaultVolatile.sol#L54
3. You are allowed to change it as often as you want. You are even allowed to change it while it is currently being changed.

Have you found a way to break the above assumptions?

[ololade97]

I totally agree with you. But the issue is the use of ">=" below.

require(targetTime >= block.timestamp + MIN_ADJUSTMENT_TIME); // dev: targetTime must be more than MIN_ADJUSTMENT_TIME in the future.

It should be ">" only and not ">=".

[reednaa]

The difference here is a single second. It doesn't make any difference in the grand scheme of things. And >= is also intended. The reason why it differs from the "usual" check is that this is a require not a if(x) revert.

[ololade97]

If >= is intended, then comment in the code is misleading. must was even used in the comment. This should be fixed.

Is there a single second difference? MIN_ADJUSTMENT_TIME is in days.

[reednaa]

must was even used in the comment. This should be fixed.

Sorry, my background is as a mathematician. > and >= are for most purposes identical. Generally, the only times when it matters is when the words strictly -> > and weakly -> >= are used. Must is just an expression of the require(...).

Is there a single second difference? MIN_ADJUSTMENT_TIME is in days.

In Solidity, when we write 7 days it is compiled as 7 * (24*60*60) since block.timestamp is in seconds.

[ololade97]

I agree with you. Yet, the comment is wrong dev: targetTime must be more than MIN_ADJUSTMENT_TIME in the future. since >= is intended.