Description

Plume Network intends to replace the Timelock contract with a governance-owned EOA. In order to implement this change, these calls into the timelock were removed in PositionUtils:

-ITimelock(timelock).enableLeverage(_vault);
+// ITimelock(timelock).enableLeverage(_vault);
 IRouter(_router).pluginIncreasePosition(_account, _collateralToken, _indexToken, _sizeDelta, _isLong);
-ITimelock(timelock).disableLeverage(_vault);
+// ITimelock(timelock).disableLeverage(_vault);

However, not all calls into the timelock were removed. In BasePositionManager, the timelock is invoked in order to read the margin-fee basis points to emit referral events:

emit IncreasePositionReferral(
    _account,
    _sizeDelta,
    ITimelock(timelock).marginFeeBasisPoints(),
    referralCode,
    referrer
);

emit DecreasePositionReferral(
    _account,
    _sizeDelta,
    ITimelock(timelock).marginFeeBasisPoints(),
    referralCode,
    referrer
);

Additionally, in PositionManager, the timelock is called before and after position increases, decreases, and liquidations.

ITimelock(timelock).enableLeverage(_vault);
IVault(_vault).liquidatePosition(_account, _collateralToken, _indexToken, _isLong, _feeReceiver);
ITimelock(timelock).disableLeverage(_vault);

ITimelock(timelock).enableLeverage(_vault);
IOrderBook(orderBook).executeIncreaseOrder(_account, _orderIndex, _feeReceiver);
ITimelock(timelock).disableLeverage(_vault);

ITimelock(timelock).enableLeverage(_vault);
IOrderBook(orderBook).executeDecreaseOrder(_account, _orderIndex, _feeReceiver);
ITimelock(timelock).disableLeverage(_vault);

Impact

Since the timelock address will be an EOA, all of these calls will revert, either on the EXTCODESIZE check emitted by the Solidity compiler or on the return-value type check. So, these features on the PositionManager and BasePositionManager will not be usable.

Recommendations

We recommend removing these calls.

Alternatively, we recommend deploying the Timelock contract as the base GMX V1 expects. Even though the forked protocol does not require a timelock, because of the tight integration between the governance timelock and the rest of the code, directly removing the timelock will deploy the project in a changed, and therefore un--battle-tested, state. On the other hand, if the Timelock contract itself is deployed as is, the buffer can be set to zero and never increased, which functionally bypasses the timelock feature of the Timelock. Although cumbersome, this would mean that no changes are necessary to implement this change where the timelock is removed.

Remediation

This issue has been acknowledged by Plume Network, and a fix was implemented in commit af802121↗.