bunker.finance contest - Picodes's results

[L - 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 TransparentUpgradeableProxy 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); In your case you have this function for example https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L274 that someone could use.
2. Scammers and malicious users may use contracts that are verified on etherscan.io and other block explorers.

Recommendations

Ensure that the initialize() function 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 {}

[NC - 01] siezeAllowedResult -> seizeAllowedResult

Here looks like there is a typo: https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L89

[NC - 02] Use NatSpec

Although the contracts are forked from compound, they should still be properly commented and you should use https://docs.soliditylang.org/en/v0.8.13/natspec-format.html. As stated: "It is recommended that Solidity contracts are fully annotated using NatSpec for all public interfaces (everything in the ABI)."

Examples: https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L83

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L166

1 - It would make code more readable for any dev

2 - There is now some powerful integrations using NatSpec: for example etherscan when checking public functions: https://etherscan.io/token/0x39aa39c021dfbae8fac545936693ac917d5e7563#writeContract

[NC - 03] Remove virtual keywords if contract is intended as final

CNft is not supposed to be inherited or to be abstract so virtual keywords should be removed. Reference: https://docs.soliditylang.org/en/v0.8.13/contracts.html#function-overriding

Example:

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L242

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L222

[NC - 04] Unused commented lines

Remove useless commented lines such as https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L241

Why don't you use 3 different contracts instead of CNft

Although the idea of having only one CNft contract is nice, why don't you do 3 versions: CNft-721, CNft-1155 and CNft-Punk. Deployment of implementations would be more costly (but just 2 contracts) and it would save your users gas as a lot of SLOAD and if else instructions could be avoided.

comptroller

Assuming you'll be using the same Comptroller across your markets, gas could be saved by passing it as a constant in your CNft` implementation. It is not settable anyway.

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNftStorage.sol#L13