Timelock.sol modification removes logic checks

[code423n4]

Handle

sirhashalot

Vulnerability details

Impact

The executeTransaction() function in Timelock.sol has been modified. Several require statements are in an if statement that is only run if admin_initialized \== true. If admin_initialized != true, these require statements will not be run. This allows for a transaction to be executed even if the "eta" time is not met. There does not appear to be a direct attack path here and the function is protected by require(msg.sender == admin), but there does not appear to be a good reason to skip these validation checks that are in the original Compound Timelock.sol.

Proof of Concept

Line 121 of Timelock.sol has an if statement branch:

if (admin_initialized) {
    require(queuedTransactions[txHash], "Tx hasn't been queued");
    require(getBlockTimestamp() >= eta, "Not surpassed timelock");
    require(getBlockTimestamp() <= eta.add(GRACE_PERIOD), "Transaction is stale");
    delete queuedTransactions[txHash];
}

This code is only run if admin_initialized == true. If that is not the case, the function will skip these validation checks. The "eta" input parameter would be ignored in this case. There does not appear to be a good reason to skip validation checks that are always run in the original Compound Timelock.sol code.

Recommended Mitigation Steps

Remove the "if" statement so that the require statements are run every time that the executeTransaction() function is run in order to avoid any unforeseen login edge cases. Compound's code is considered quite secure as it is, but modifying it and removing validation checks is not recommended.

[0xleastwood]

I mostly agree with the warden here. It doesn't really make a lot of sense why executeTransaction needs to allow for instant processing of proposals. Could you explain? @ColaM12

[0xleastwood]

I think I can see a usecase where the admin would want to be able to process transactions instantly. I'd imagine the admin would be initialized after setup.

[0xleastwood]

I'm gonna mark this as non-critical as it seems like there is a valid reason as to why this is necessary. This allows for the admin to setup the protocol seamlessly.

[jimmy-openlev]

Agreed with the judge. This check makes the upgrade process efficient before transferring protocol control to OpenLeverage DAO. This check will be effective once the tokens and DAO are launched.