tsvetanovv - There is no way to decrease timelock delay

[sherlock-admin]

tsvetanovv

medium

There is no way to decrease timelock delay

Summary

In Timelock.sol we have increaseTimelockDelay().

56: function increaseTimelockDelay(uint256 _timelockDelay) external onlyTimelockAdmin nonReentrant {
57:        if (_timelockDelay <= timelockDelay) { 
58:            revert InvalidTimelockDelay(_timelockDelay);
59:        }
60:
61:        if (_timelockDelay > MAX_TIMELOCK_DELAY) {
62:            revert MaxTimelockDelayExceeded(_timelockDelay);
63:        }
64:
65:        timelockDelay = _timelockDelay;
66:    }

This function increase the timelock delay, but Timelock.sol has no functionality to decrease timelock delay.

Vulnerability Detail

In Timelock.sol in the constructor we set timelock delay.

44: timelockDelay = _timelockDelay;

Timelock admin can then use the increaseTimelockDelay() function to increase the timelockDelay to a maximum of 5 days.

21: uint256 public constant MAX_TIMELOCK_DELAY = 5 days;

But once admin set timelockDelay on 5 days it can't be change because the first 2 if checks stop him from changing it.

57:        if (_timelockDelay <= timelockDelay) { 
58:            revert InvalidTimelockDelay(_timelockDelay);
59:        }
60:
61:        if (_timelockDelay > MAX_TIMELOCK_DELAY) {
62:            revert MaxTimelockDelayExceeded(_timelockDelay);
63:        }

Apart from all these things, there is also a possibility that timelockDelay is set to more than 5 days by mistake in the constructor, because there is no check.

Impact

Once timelockDelay is set to 5 days it cannot be changed anymore.

Code Snippet

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/config/Timelock.sol#L56-L66

56: function increaseTimelockDelay(uint256 _timelockDelay) external onlyTimelockAdmin nonReentrant {
57:        if (_timelockDelay <= timelockDelay) { 
58:            revert InvalidTimelockDelay(_timelockDelay);
59:        }
60:
61:        if (_timelockDelay > MAX_TIMELOCK_DELAY) {
62:            revert MaxTimelockDelayExceeded(_timelockDelay);
63:        }
64:
65:        timelockDelay = _timelockDelay;
66:    }

Tool used

Manual Review

Recommendation

I think the best solution to this problem is to add a function decreaseTimelockDelay(). And in addition check the timelockDelay if it does not exceed 5 days when is set in the constructor.

[cvetanovv]

Escalate for 10 usdc.

I think this is valid medium issue.

[sherlock-admin]

Escalate for 10 usdc.

I think this is valid medium issue.

You've created a valid escalation for 10 USDC!

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.

[IllIllI000]

This is a design decision, and I believe they can just deploy a new timelock and give grant it the right role if they really need to decrease it. Going over the maximum would be an admin error which Sherlock does not reward

[hrishibhat]

Escalation rejected

Not a valid high/medium

[sherlock-admin]

Escalation rejected

Not a valid high/medium

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.