Canto contest - k's results

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/NoteInterest.sol#L118

Vulnerability details

Impact

updateBaseRate() was a publicly accessible function, which allowed any user to set an arbitrary value for the baseRatePerYear variable after a 24 hour delay from the last time it was set. This value was used in the calculation of the getBorrowRate() and getSupplyRate() functions, which in turn influenced rates for Notes. The issue was simple to identify and execute, but given that the value could be reset by an admin calling the _setBaseRatePerYear() function and the requirement of a 24 hour delay, the issue was rated as a medium.

Proof of Concept

N/A

Tools Used

N/A

Recommended Mitigation Steps

The intent behind the function was not internally clear. If the intention was to users to control the function, some validation should be put in place to ensure the value is within certain bounds (e.g. min-max bounds or within a certain percentage of the last value set). If the function should not be publicly accessible, access control should be added to ensure only specific accounts can call the function.

1. Common practice in Solidity is to prefix internal and private functions with an underscore (e.g. _internalFunction()). Usage of underscore naming was unconventional and inconsistent throughout the codebase. For example, in Comptroller.sol functions prefixed with underscores included:
   • _setPriceOracle - public function
   • _setCloseFactor - external function
   • _addMarketInternal - internal function It is recommended that a consistent naming pattern is used for functions to improve with readability and communicate clearly what the visibility of the function should be.
2. Parts of the codebase were still a work in progress. Several TODO comments (e.g. 1 and unused variables (e.g. 2) were found throughout the codebase, which made it difficult to establish what was and wasn’t intended functionality.
3. The following typos were identified:
   • canceled → cancelled for proposal type. This should be changed in the struct, as well as the comments and events that reference it https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoInterfaces.sol#L102 and in events and comments
   • arity → parity https://github.com/Plex-Engineer/lending-market/blob/755424c1f9ab3f9f0408443e6606f94e4f08a990/contracts/Governance/GovernorBravoDelegate.sol#L45

Unnecessary usage of assert

BaseV1-periphery.sol#L82 made use of assert in the fallback function to ensure the caller was only ever the WETH contract. As assert should only be used to examine invariants and test for internal problems, the use of assert here appears to be inappropriate as a user may accidentally call into the fallback function when interacting with the contract.

Unnecessary usage of safe math functions

The GovernorBravoDelegate contract made use of Solidity ^0.8.10, while also using add256() and sub256() functions to validate for under- and overflows. As the operations were not delimited by unchecked blocks, it was safe to perform the operations without using the safe function equivalents - which wasted gas.

Use custom errors instead of require messages

require statements were used throughout the codebase. Replacing all require statements with custom errors will reduce the gas cost of deployment, as well as runtime costs when the revert condition is met.