bunker.finance contest - hyh's results

1. NFT oracle can be set to zero (low)

Core collateral valuation logic depends on a valid oracle, functionality of the system will be unavailable if the oracle is set to zero (there will be low level reverts).

Proof of Concept

nftOracle value isn't controlled to be valid:

_setNftPriceOracle:

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L764-L771

    function _setNftPriceOracle(NftPriceOracle newOracle) public returns (uint) {
        // Check caller is admin
        if (msg.sender != admin) {
            return fail(Error.UNAUTHORIZED, FailureInfo.SET_PRICE_ORACLE_OWNER_CHECK);
        }

        // Set comptroller's nft oracle to newOracle
        nftOracle = newOracle;

_initializeNftCollateral:

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

nftOracle = _nftOracle;

Both functions are admin only, but nftOracle can be omitted by mistake.

Recommended Mitigation Steps

Consider adding zero address checks

2. One step admin change (non-critical)

One step process offers no protection for the cases when admin rights transfer is performed mistakenly or with any malicious intent.

Adding a modest complexity of an additional step and a delay is a low price to pay for having time to evaluate the change.

Proof of Concept

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Comptroller.sol#L731-L734

    function _changeAdmin(address newAdmin) external {
        require(msg.sender == admin, "Only admin can change the admin");
        admin = newAdmin;
    }

Recommended Mitigation Steps

Consider utilizing two-step admin rights transferring process (proposition and acceptance in the separate actions) with a noticeable delay between the steps to enforce the transparency and stability of the system.

3. Floating pragma is used across the system (non-critical)

As different compiler versions have critical behavior specifics if the contracts get accidentally deployed using another compiler version compared to the one they were tested with, various types of undesired behavior can be introduced.

Proof of Concept

Old contracts use pragma solidity ^0.5.16:

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

New ones use pragma solidity ^0.8.0:

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

Recommended Mitigation Steps

As this Compound fork was developed and tested with some stable version of compiler it's adviced to directly fixing it for the whole set of system contracts.

4. In mint() user balance is increased before actual transfer of the funds (non-critical)

As the tokens here are IERC1155 and IERC721 that have transfer hook, this would be immediately exploitable with the whole contract balance as the target in absence of nonReentrant modifier.

Now suggesting to move it after transfers as a best practice.

Proof of Concept

totalBalance is increased before assets have changed hands:

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

totalBalance[msg.sender] += totalAmount;

Recommended Mitigation Steps

Move balance increase to happen after fund transfer:

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

totalBalance[msg.sender] += totalAmount;
_mintBatch(msg.sender, tokenIds, amounts, "");

5. Misspelling in comment (non-critical)

Should be 'instead':

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

// We call the internal function instad of the public one because in liquidation, we

6. Asserts used instead of require (non-critical)

Assert will consume all the available gas, providing no additional benefits when being used instead of require, which both returns gas and allows for error message.

Proof of Concept

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

assert(assetIndex < len);

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

assert(markets[cToken].accountMembership[borrower]);

Recommended Mitigation Steps

Using assert in production isn't recommended, consider substituting it with require in all the cases.