QA Report

[code423n4]

Missing Equivalence Checks in Setters

Severity: Low
Context: VestedEscrow.sol#L64-L72, VestedEscrow.sol#L74-L78

Description:
Setter functions are missing checks to validate if the new value being set is the same as the current value already set in the contract. Such checks will showcase mismatches between on-chain and off-chain states.

Recommendation:
Add in the additional checks to validate if the new value being set is the same as the current value already set in the contract.

Missing Time locks

Severity: Low
Context: Controller.sol#L62-L76

Description:
None of the onlyOwner functions that change critical protocol addresses/parameters appear to have a time lock for a time-delayed change to alert: (1) users and give them a chance to engage/exit protocol if they are not agreeable to the changes (2) team in case of compromised owner(s) and given them a chance to perform incident response.

Recommendation:
Add a time lock to these functions for a time-delayed change to alert users and protect against possiable malicious changes by compromised owners(s).

Lack of Event Emission For Critical Functions

Severity: Low
Context: Controller.sol#L33-L37, StakerVault.sol#L98-L102, StakerVault.sol#L197-L210, StakerVault.sol#L218-L235, AmmGauge.sol#L49-L54, InflationManager.sol#L58-L63, InflationManager.sol#L435-L438, InflationManager.sol#L446-L467, InflationManager.sol#L482-L489, KeeperGauge.sol#L57-L62, Minter.sol#L99-L102, Minter.sol#L104-L108, VestedEscrow.sol#L64-L72, VestedEscrow.sol#L74-L78

Description:
Several functions update critical parameters that are missing event emission. These should be performed to ensure tracking of changes of such critical parameters.

Recommendation:
Add events to functions that change critical parameters.

Max/Infinite Approvals are Dangerous

Severity: Low
Context: RewardHandler.sol#L62-L65

Description:
Giving max/infinite approvals to contracts are dangerous. Giving max/infinite approvals to contracts are dangerous because if those contracts are exploited then they can remove all the funds from the approving addresses.

Recommendation
Check allowance and approve as much as required.

TODOs Left In The Code

Severity: Informational
Context: InflationManager.sol#L532

Description:
There should never be any TODOs in the code when deploying.

Recommendation:
Finish the TODOs before deploying.

Spelling Errors

Severity: Informational
Context: BkdLocker.sol#L173 (invlude => include), FeeBurner.sol#L29 (successfull => successful), FeeBurner.sol#L29 (Emmited => Emitted), FeeBurner.sol#L35 (Recieve => Receive), FeeBurner.sol#L84 (Transfering => Transferring)

Description:
Spelling errors in comments can cause confusion to both users and developers.

Recommendation:
Check all misspellings to ensure they are corrected.

Missing or Incomplete NatSpec

Severity: Informational
Context: All Contracts

Description:
Some functions are missing @notice/@dev NatSpec comments for the function, @param for all/some of their parameters and @return for return values. Given that NatSpec is an important part of code documentation, this affects code comprehension, auditability and usability.

Recommendation:
Add in full NatSpec comments for all functions to have complete code documentation for future use.

[GalloDaSballo]

Missing Equivalence Checks in Setters

Not sure if this should be gas as it would increase cost of operations, personally am ambivalent on implementing and would consider non-critical

Missing Time locks

Disagree as you cannot prove the presence or absence of timelock, if anything you could recommend using it, and yet no proof would be in the code, making the finding unactionable

Lack of Event Emission For Critical Functions

Disagree with the alarmism, agree with the informational finding

Max/Infinite Approvals are Dangerous

Valid but not actionable in lack of an alternative

TODOs Left In The Code, Spelling Errors

Valid non-critical finding

Missing or Incomplete NatSpec

I'm very confident you're not supposed to always mark every variable as sometimes the best comment is the one you don't write.

Personally the report feels really cookie cutter, perhaps automated