Add a timelock to `setDefaultFeePercentage()`,`setCustomFeePercentageForCollateral()`, `setCustomFeePercentageForSafe()` and `setMinDebtPercentageForSaving()`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboClerk.sol#L36-L44 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboClerk.sol#L70-L78 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboClerk.sol#L88-L96 https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboSavior.sol#L75-L83

Vulnerability details

Impact

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate.

Here, no timelock capabilities seem to be used:

src/modules/TurboClerk.sol:36:    function setDefaultFeePercentage(uint256 newDefaultFeePercentage) external requiresAuth {
src/modules/TurboClerk.sol:70:    function setCustomFeePercentageForCollateral(ERC20 collateral, uint256 newFeePercentage) external requiresAuth {
src/modules/TurboClerk.sol:88:    function setCustomFeePercentageForSafe(TurboSafe safe, uint256 newFeePercentage) external requiresAuth {
src/modules/TurboSavior.sol:75:    function setMinDebtPercentageForSaving(uint256 newMinDebtPercentageForSaving) external requiresAuth {

I believe those are should've been governance functions, impacting multiple users enough to make them want to react / be notified ahead of time.

Proof of Concept

The requiresAuth modifier and the above image don't imply that the mentioned functions are behind a timelock.

File: TurboClerk.sol
36:     function setDefaultFeePercentage(uint256 newDefaultFeePercentage) external requiresAuth {
37:         // A fee percentage over 100% makes no sense.
38:         require(newDefaultFeePercentage <= 1e18, "FEE_TOO_HIGH");
39: 
40:         // Update the default fee percentage.
41:         defaultFeePercentage = newDefaultFeePercentage;
42: 
43:         emit DefaultFeePercentageUpdated(msg.sender, newDefaultFeePercentage);
44:     }

File: TurboClerk.sol
70:     function setCustomFeePercentageForCollateral(ERC20 collateral, uint256 newFeePercentage) external requiresAuth {
71:         // A fee percentage over 100% makes no sense.
72:         require(newFeePercentage <= 1e18, "FEE_TOO_HIGH");
73: 
74:         // Update the custom fee percentage for the Safe.
75:         getCustomFeePercentageForCollateral[collateral] = newFeePercentage;
76: 
77:         emit CustomFeePercentageUpdatedForCollateral(msg.sender, collateral, newFeePercentage);
78:     }

File: TurboClerk.sol
88:     function setCustomFeePercentageForSafe(TurboSafe safe, uint256 newFeePercentage) external requiresAuth {
89:         // A fee percentage over 100% makes no sense.
90:         require(newFeePercentage <= 1e18, "FEE_TOO_HIGH");
91: 
92:         // Update the custom fee percentage for the Safe.
93:         getCustomFeePercentageForSafe[safe] = newFeePercentage;
94: 
95:         emit CustomFeePercentageUpdatedForSafe(msg.sender, safe, newFeePercentage);
96:     }

File: TurboSavior.sol
75:     function setMinDebtPercentageForSaving(uint256 newMinDebtPercentageForSaving) external requiresAuth {
76:         // A minimum debt percentage over 100% makes no sense.
77:         require(newMinDebtPercentageForSaving <= 1e18, "PERCENT_TOO_HIGH");
78: 
79:         // Update the minimum debt percentage.
80:         minDebtPercentageForSaving = newMinDebtPercentageForSaving;
81: 
82:         emit MinDebtPercentageForSavingUpdated(msg.sender, newMinDebtPercentageForSaving);
83:     }

Recommended Mitigation Steps

Consider adding a timelock to setDefaultFeePercentage(),setCustomFeePercentageForCollateral(), setCustomFeePercentageForSafe() and setMinDebtPercentageForSaving()

[Joeysantoro]

All of these will be behind timelocks at the Authority level. Given this is a configuration consideration I am disputing

[GalloDaSballo]

I have to agree with the sponsor here, the functions mentioned are clearly permissioned, however no exploit or vulnerability has been shown in this finding. The recommendation of using a timelock is welcome (and the sponsor has already made it clear they will), however in lack of any vulnerability, am going to mark the finding invalid