kenzo - Ineffective timelocker due to epoch length

[sherlock-admin]

kenzo

medium

Ineffective timelocker due to epoch length

Summary

The timelocker has a timelock of 3 days, while the epochs length is 7 or 30 days.
Users can not withdraw their funds after an epoch has started.
This renders the timelock ineffective, as users can not withdraw their funds while a malicious timelock transaction is pending,
and the admin can rug the protocol.

Vulnerability Detail

The Y2K docs state that epochs locking period will be weekly and monthly.
The contest readme states that Admin Should not be able to steal user funds.
To mitigate against such risk, a timelocker has been added.
It sets the waiting period as such:

    uint32 public constant MIN_DELAY = 3 days;
    uint32 public constant MAX_DELAY = 30 days;
    uint32 public constant GRACE_PERIOD = 14 days;
    function queue(
        ...
        if (
            _timestamp < block.timestamp + MIN_DELAY ||
            _timestamp > block.timestamp + MAX_DELAY
        ) {
            revert TimestampNotInRangeError(block.timestamp, _timestamp);
        }

So a transaction has to wait for a minimum of 3 days.
But since epochs last for a week or a month (as per the documentation),
a user that has locked his funds for such a period can not withdraw them in the 3 days of timelock.
He can not effectively do anything to save his funds in case of malicious or compromised admin.

The contest readme states that Admin Should not be able to steal user funds.
But for example an admin can rug in the following way:
Once an epoch has started, queue a tx that whitelists his own address for the vault (whitelistAddressOnMarket), then queue a tx that whitelists his EOA as controller (whitelistController), then queue a tx that changes the controller to his EOA (changeController), and after 3 days, he'll execute them all, and use his EOA to call sendTokens in the vault and send himself all the user funds.

Impact

Timelock is not effective.
Admin can rug user funds, which according to the contest readme, should not be possible.

Code Snippet

    uint32 public constant MIN_DELAY = 3 days;
    uint32 public constant MAX_DELAY = 30 days;
    uint32 public constant GRACE_PERIOD = 14 days;
    function queue(
        ...
        if (
            _timestamp < block.timestamp + MIN_DELAY ||
            _timestamp > block.timestamp + MAX_DELAY
        ) {
            revert TimestampNotInRangeError(block.timestamp, _timestamp);
        }

Tool used

Manual Review

Recommendation

I believe that for the timelock to be effective, it has to have a minimum delay which is longer than the epoch's duration.

[3xHarry]

considering this

[3xHarry]

fix PR: Y2K-Finance/Earthquake#138

[shaka0x]

Escalate for 10 USDC.

The README makes a clear distinction between Admin and Timelock:
Admin can configure new markets and epochs on those markets, Timelock can make cirital changes like changing the oracle or whitelisting controllers.

So the statement Admin Should not be able to steal user funds does not apply for this case.

[sherlock-admin]

Escalate for 10 USDC.

The README makes a clear distinction between Admin and Timelock:
Admin can configure new markets and epochs on those markets, Timelock can make cirital changes like changing the oracle or whitelisting controllers.

So the statement Admin Should not be able to steal user funds does not apply for this case.

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.

[KenzoAgada]

Escalate for 10 USDC

The previous escalator's escalation reasoning seems wrong to me and I'd like to explain why.

Escalate for 10 USDC.

The README makes a clear distinction between Admin and Timelock: Admin can configure new markets and epochs on those markets, Timelock can make cirital changes like changing the oracle or whitelisting controllers.

So the statement Admin Should not be able to steal user funds does not apply for this case.

Even if the statement "Admin Should not be able to steal user funds" does not apply for this case is true,
(and I'm not sure if the distinction between Admin and Timelock is as clear cut as the escalator wrote),
do we anyway think that the Timelock should be able to steal user funds?

The project has added a timelock to add a guarantee to users that their funds can not be stolen via changing parameters like the Controller or Oracle.
Obviously a timelock of 1 second would be ineffective;
Similarly, a timelock of 3 days, when the user funds are locked for 7 or 15 days, is also ineffective.
User funds can still be stolen all the same by a compromised/malicious account. The timelock almost does nothing in this case, the implementation is faulty.

I can imagine the Twitter threads if this was to be exploited... "The sensitive functions were kept behind a timelock, but the timelock's duration was shorter than the amount of time that user funds were locked. So users could do nothing and only watch while they waited for their assets to be stolen".

We can also see that the sponsor confirmed the issue and fixed it.

Therefore, I believe a medium severity is appropriate.

[sherlock-admin]

Escalate for 10 USDC

The previous escalator's escalation reasoning seems wrong to me and I'd like to explain why.

Escalate for 10 USDC.

The README makes a clear distinction between Admin and Timelock: Admin can configure new markets and epochs on those markets, Timelock can make cirital changes like changing the oracle or whitelisting controllers.

So the statement Admin Should not be able to steal user funds does not apply for this case.

Even if the statement "Admin Should not be able to steal user funds" does not apply for this case is true,
(and I'm not sure if the distinction between Admin and Timelock is as clear cut as the escalator wrote),
do we anyway think that the Timelock should be able to steal user funds?

The project has added a timelock to add a guarantee to users that their funds can not be stolen via changing parameters like the Controller or Oracle.
Obviously a timelock of 1 second would be ineffective;
Similarly, a timelock of 3 days, when the user funds are locked for 7 or 15 days, is also ineffective.
User funds can still be stolen all the same by a compromised/malicious account. The timelock almost does nothing in this case, the implementation is faulty.

I can imagine the Twitter threads if this was to be exploited... "The sensitive functions were kept behind a timelock, but the timelock's duration was shorter than the amount of time that user funds were locked. So users could do nothing and only watch while they waited for their assets to be stolen".

We can also see that the sponsor confirmed the issue and fixed it.

Therefore, I believe a medium severity is appropriate.

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.

[hrishibhat]

Escalation accepted

Valid low
Although the point raised in the issue could point to a possible malicious maneuver, Timelocker is trusted to make critical parameter changes. Also, this is a tradeoff as setting longer timelock periods for longer epochs is not practical.

Sponsor comment:

well we do have monthly epochs where 7 days is def not enough and the timelock is able to influence epoch settlement mid epcoh, however most epochs are weekly epochs. However as the timelock is controlled by dao msig, the assumption is that users trust dao actions and the delay is more of a transparency mechanic.

[sherlock-admin]

Escalation accepted

Valid low
Although the point raised in the issue could point to a possible malicious maneuver, Timelocker is trusted to make critical parameter changes. Also, this is a tradeoff as setting longer timelock periods for longer epochs is not practical.

Sponsor comment:

well we do have monthly epochs where 7 days is def not enough and the timelock is able to influence epoch settlement mid epcoh, however most epochs are weekly epochs. However as the timelock is controlled by dao msig, the assumption is that users trust dao actions and the delay is more of a transparency mechanic.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.