code-423n4 / 2022-05-bunker-findings

1 stars 0 forks source link

QA Report #68

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

[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

bunkerfinance-dev commented 2 years ago

This report was useful to us.