Canto contest - Picodes's results

Lines of code

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

Vulnerability details

Impact

In NoteRateModel, the function updateBaseRate has no access control. Therefore anyone could potentially call it and set baseRatePerYear to an incorrect value, to extract funds from borrowers or drop borrowing cost for an attack.

Proof of Concept

An attacker could set baseRatePerYear to an arbitrarily high value close to max uint, which would lead to all Notes borrowers paying extremely high interests and losing funds before the admin could react.

Recommended Mitigation Steps

Add a whitelisting requirement or an admin privilege for this function.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/ERC20.sol#L33 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/ERC20.sol#L95

Vulnerability details

Impact

For an obscure reason as it’s not commented, _totalSupply is not initialized to 0, leading to an inaccurate total supply, which could easily break integrations, computations of market cap, etc.

Proof of Concept

If the constructor is called with _initialSupply = 1000, then 1000 tokens are minted. The total supply will be 2000.

Recommended Mitigation Steps

Remove _initialSupply.

Lines of code

https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Note.sol#L18

Vulnerability details

Impact

In Note, totalSupply could overflow, preventing the accountant to mint and work as intended.

Proof of Concept

As type(uint).max Note are minted in the accountant, totalSupply can overflow if it’s not 0 before the mint. But in the constructor, totalSupply_ could be non 0.

Recommended Mitigation Steps

Remove totalSupply_ from the constructor.

1 low and 5 non-critical issues.

[Low - 01] - ERC20 should be flagged as abstract

ERC20 is clearly not to be used directly as it does not implement mint or burn. This should specified somehow, either with a clear comment or by making the contract abstract.

[NC - 01] - ProposalStore - Incorrect Comment

Here, the comment is not accurate and does not make sense.

[NC - 02] - ProposalStore - Incorrect Comment

Here the comment does not correspond to the Proposal struct.

[NC - 03] - ProposalStore - UniGovModAcct and OnlyUniGov are useless

What are UniGovModAcct and OnlyUniGov used for in this contract ? Either they could be removed, either you should add a comment explaining their purpose.

[NC - 04] - Respect naming conventions

For example here, the function name begins with _ which usually means the function is internal.

[NC - 05] - Comment the code and follow the NatSpec standard

Numerous contracts, like this one or this one are not commented. If you intend to make this code public, you should have a clear documentation.

Reference: https://docs.soliditylang.org/en/develop/natspec-format.html

[Gas - 01] - Useless functions

https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Note.sol#L21 https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/Note.sol#L6

accountant is public, so there is no need for a view function as you already have the getter.

[Gas - 02] - Useless hook

This line https://github.com/Plex-Engineer/lending-market/blob/ab31a612be354e252d72faead63d86b844172761/contracts/ERC20.sol#L95 is useless as if the totalSupply is 0 it means all the tokens have been burned, so transfers will revert anyway as all the balances are null. Furthermore how can totalSupply be 0 as one cannot burn more than what has been minted ?

It could therefore be removed to save gas on each transfer.