Canto v2 contest - Picodes's results

Lines of code

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

Vulnerability details

Impact

_totalSupply can be initialized to something different than 0, which would lead to an inaccurate total supply, and could easily break integrations, computations of market cap, etc.

Proof of Concept

If the constructor is called with _initialSupply = 1000, the totalSupply will be initialized to 1000.

As all the others computations are correct, there will be for ever a discrepancy of 1000 between the real total supply and the one of the contract.

Recommended Mitigation Steps

Remove _initialSupply.

Lines of code

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

Vulnerability details

Impact

The ensure modifier is commented, so deadlines will not work when passing orders, breaking this functionnality.

Lines of code

https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/CNote.sol#L33 https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Note.sol#L14

Vulnerability details

Impact

The fact that there is no cap on the amount of Note that can be borrowed makes the Oracle Extractable Value unlimited. But as you intend to rely on TWAP, you need to make sure the cost of oracle manipulation is lower than the Oracle Extractable Value.

Proof of Concept

By manipulating the TWAPs of the designated proxy used for Note (USDC ?) and its relative price to a given collateral(which would be highly costly), an attacker could borrow Note without limit, and empty all pools related to Note and all Note-related value, instantly killing the stablecoin.

The value extractable by Oracle Manipulations is usually easily computable as it is the size of the lending market, but here, it’s more difficult to evaluate as it could potentially be any value linked to Note. This makes risk management harder and increase significantly the risk of attack.

Therefore a cap on how many Notes can be borrowed needs to be added to mitigate this risk.

The attack would be:

• Manipulate the USDC / Collateral TWAP to be able to borrow Note with less than 1$ of collateral, which would be costly.
• Extract all the value possible linked to Note, for example: - by buying all the tokens from pools Note / token at a discount - by supplying Notes to the lending platform and borrow collaterals for which the Note price is still at 1$ - etc

Essentially as you have no cap on the amount of Note that could be borrowed in such a scenario, you cannot be sure that the potential attack profits are lower than the attack cost.

Recommended Mitigation Steps

The governance needs to set a limit on how much Note can be borrowed to mitigate risks, or add for example an “hourly” borrowing limit.

Easiest way to do this would be able to mint / burn from the accountant

        //set price statically to 1 when the Comptroller is retrieving Price
        else if (compareStrings(ctoken.symbol(), "cNOTE") && msg.sender == Comptroller) {
            return 1; // Note price is fixed to 1
        }

        else {
            stablePair = (stable == 0) ? false : true;
            pair = IBaseV1Pair(pairFor(USDC, underlying, stablePair)); //get the pair for the USDC/underlying pool
            price = pair.quote(underlying, 1, 8); //how much USDC is this token redeemable for
        }

[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

[Gas - 01] - 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.

[Gas - 02] Use Custom Errors

Instead of using error strings, to reduce deployment and runtime cost, you should use Custom Errors: see https://blog.soliditylang.org/2021/04/21/custom-errors/. This would save both deployment and runtime cost.

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

[Gas - 03] Using address to test equality would be more efficient

Instead of doing https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L495 or https://github.com/Plex-Engineer/lending-market-v2/blob/ea5840de72eab58bec837bb51986ac73712fcfde/contracts/Stableswap/BaseV1-periphery.sol#L500, it’d be more efficient to have setters and test that addresses are the same as it would avoid an external call.