QA Report

[code423n4]

[Low - 01] - Make sure to also initialize implementations when using upgradeable contracts

When using an upgradeable proxy over an implementation it is important to ensure the underlying implementation is initialised in addition to the proxied contract.

Consider the following example where we have a Proxy as contractA and a Implementation as contractB . As a parameter to the constructor of contractA the data is provided to make a delegate call to contractB.initialize() which updates the storage as required in contractA .

Thus, contractB has not been initialized, only contractA has had its state updated. As a result any user is able to call initialize() on contractB , giving themselves full permissions over the contract. There are two reasons why it is not desirable for malicious users to have full control over a contract:

1. Any delegate calls to external contracts can call selfdestruct which would delete the implementation contract temporarily making the proxy unusable (until it can be updated with a new implementation); This is not the case for your implementations from what I’ve seen (otherwise this would have been a high issue).
2. Scammers and malicious users may use implementation contracts, and you’d rather avoid this and keep control over them..

Recommendations

Ensure that the initializer modifier is called on all underlying implementations during deployment. This could easily be done by adding in the implementation:

// @custom:oz-upgrades-unsafe-allow constructor
constructor() initializer {}

[Low - 02] - Use storage gaps instead of separating the code from the storage

Instead of the old-fashioned, Compound way of coding upgradeable contracts (with XXStorageV1 that you then use as a child for XXStorageV2), it’d be cleaner to use the OpenZeppelin way, were the storage is were it belongs and you just add storage gaps to contracts.

This holds for TreasuryDelegator and AccountantDelegator. Reference: https://medium.com/nomic-foundation-blog/malicious-backdoors-in-ethereum-proxies-62629adf3357

[Low - 03] - Proxies are subject to Proxy selector clashing

This holds for TreasuryDelegator, AccountantDelegator and the others Delegators. Reference: https://medium.com/nomic-foundation-blog/malicious-backdoors-in-ethereum-proxies-62629adf3357

[NC - 01] - Use type(uint256).max

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/ERC20.sol#L18

For readability you can use type(uint256).max instead of defining uint256 MAX_INT = 2**256-1;

[NC - 02] - ERC20: make decimals virtual

According to your comments decimals is intended to be potentially overloaded. Therefore make it virtual.

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/ERC20.sol#L65

[NC - 03] - Correct indentation

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/ERC20.sol#L92

[NC - 04] - Useless variable

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/ERC20.sol#L13

_initialSupply is not used anywhere so could be removed.

[NC - 05] - Import of hardhat.console

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Accountant/AccountantDelegate.sol#L7 https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CErc20.sol#L5 https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-core.sol#L4

[NC - 06] - Remove payable in fallback

Instead of using a custom require, why not just removing the payable modifier in the following fallbacks ?

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Accountant/AccountantDelegator.sol#L123 https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Treasury/TreasuryDelegator.sol#L121

[NC - 07] - In Delegator contracts, a lot of functions are useless and could just be remove: the fallback is enough

Examples: https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Treasury/TreasuryDelegator.sol#L63 https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Treasury/TreasuryDelegator.sol#L67

[NC - 08] - Typo: doesn"t -> doesn’t

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L230

[GalloDaSballo]

[Low - 01] - Make sure to also initialize implementations when using upgradeable contracts

I don't believe this applies to the codebase as the pattern: -> Base Contract with Offset -> DelegateCall -> Implementation Contract with Offset means that the storage is actually kept in BaseContract, and the Offset is used to ensure that the Implementation will not break the BaseContract (which can still happen if done improperly)

However I don't think them implementation needs initialization "in general"

[Low - 02] - Use storage gaps instead of separating the code from the storage

Similar reasoning here, I think Refactoring is more appropriate and No Gaps are necessary for the Implementations as long as they follow the Offset Pattern R

[Low - 03] - Proxies are subject to Proxy selector clashing

This is a valid finding in that a clashing implementation will never be executable as the BaseContract selector will match

L

[NC - 01] - Use type(uint256).max

R

 [NC - 02] - ERC20: make decimals virtual

Nice Catch, valid R

[NC - 03] - Correct indentation

R

[NC - 04] - Useless variable

NC

[NC - 05] - Import of hardhat.console

R

[NC - 06] - Remove payable in fallback

R

[NC - 07] - In Delegator contracts, a lot of functions are useless and could just be remove: the fallback is enough

Because of the clash mentioned above, I'm not counting this as valid, I'd assume the idea is to avoid the clashes as mentioned above

[NC - 08] - Typo: doesn"t -> doesn’t

NC

[GalloDaSballo]

Nice report, a couple copy pasta but rest is pretty unique

[GalloDaSballo]

1L 6R 2NC