Timelock has a rug vector bypass

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L295-L301

Vulnerability details

Impact

Having a bypass for the timelock defeats the purpose of a timelock. Anyone in the EMERGENCY_ROLE, or who is able to compromise the key is able to immediately make changes at the expense of users.

Proof of Concept

executeEmergency() can take any action on behalf of the owner, immediately:

File: contracts/governance/TimelockControllerEmergency.sol   #1

295       function executeEmergency(
296           address target,
297           uint256 value,
298           bytes calldata data
299       ) public payable onlyRole(EMERGENCY_ROLE) {
300           _call(0, 0, target, value, data);
301       }

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/governance/TimelockControllerEmergency.sol#L295-L301

So essentially any rug vector is immediately available, e.g the of setting fees to 100%:

File: contracts/NestedFactory.sol   #2

158       /// @inheritdoc INestedFactory
159       function setEntryFees(uint256 _entryFees) external override onlyOwner {
160           require(_entryFees != 0, "NF: ZERO_FEES");
161           require(_entryFees <= 10000, "NF: FEES_OVERFLOW");
162           entryFees = _entryFees;
163           emit EntryFeesUpdated(_entryFees);
164       }
165   
166       /// @inheritdoc INestedFactory
167       function setExitFees(uint256 _exitFees) external override onlyOwner {
168           require(_exitFees != 0, "NF: ZERO_FEES");
169           require(_exitFees <= 10000, "NF: FEES_OVERFLOW");
170           exitFees = _exitFees;
171           emit ExitFeesUpdated(_exitFees);
172       }

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L158-L172

Tools Used

Code inspection

Recommended Mitigation Steps

Only allow bypassing the timelock for specific actions, not arbitrary calls

[obatirou]

Disputed

It's about wardens appreciation of our ownership architecture versus ours. We can imagine many other malicious scenarios, assuming that the Multisig/Timelock/OwnerProxy combination is not enough to prevent the protocol from being compromised.

[jack-the-pug]

It's pretty clear that this is designed to be so.