bunker.finance contest - TerrierLover's results

CNft.sol

Consider using upgradeable counterpart interfaces

CNft.sol imports non-upgradeable version of openzeppelin interfaces.

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

import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";
import "@openzeppelin/contracts/interfaces/IERC1155.sol";
import "@openzeppelin/contracts/interfaces/IERC721.sol";
import "@openzeppelin/contracts/utils/introspection/ERC165.sol";

For the consistency, it can use the upgradeable version of interfaces. Here are upgradeable version of interfaces. It also needs to update codes to use upgradeable interface.

import "@openzeppelin/contracts-upgradeable/token/ERC721/IERC721ReceiverUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/token/ERC1155/IERC1155ReceiverUpgradeable.sol";
import "@openzeppelin/contracts-upgradeable/interfaces/IERC1155Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/interfaces/IERC721Upgradeable.sol";
import "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol";

---

Indentation of safeTransferFrom function seems not consistent

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

function safeTransferFrom(
    address from,
    address to,
    uint256 id,
    uint256 amount,
    bytes calldata data
    ) public virtual override {
        // Unused.
        from;
        to;
        id;
        amount;
        data;

        revert("CNFT: Use safeBatchTransferFrom instead");
}

The indentation of safeTransferFrom seems not consistent with other codebase.

It can write like this to be consistent.

function safeTransferFrom(
    address from,
    address to,
    uint256 id,
    uint256 amount,
    bytes calldata data
) public virtual override {
    // Unused.
    from;
    to;
    id;
    amount;
    data;

    revert("CNFT: Use safeBatchTransferFrom instead");
}

---

ERC1155Enumerable.sol

Use upgradeable counterpart instead of IERC1155.sol

IERC1155.sol is imported at ERC1155Enumerable.sol.

import "@openzeppelin/contracts/interfaces/IERC1155.sol";

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

Instead, it can use IERC1155Upgradeable.sol which is a upgradeable counterpart.

import "@openzeppelin/contracts-upgradeable/interfaces/IERC1155Upgradeable.sol";

---

CNftPriceOracle.sol

Order of modifier onlyAdmin() seems not ideal

As mentioned in the solidity document, the order of modifiers should be after state variables. https://docs.soliditylang.org/en/v0.8.13/style-guide.html#order-of-layout

Inside each contract, library or interface, use the following order:
1. Type declarations
2. State variables
3. Events
4. Modifiers
5. Functions

Modifiers is defined between admin and uniswapV2Oracle state variables, which seem not ideal. https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/CNftPriceOracle.sol#L30-L33

onlyAdmin modifier can be located after underlyingNftxTokenAddress state variable.

address public admin;
UniswapV2PriceOracle public immutable uniswapV2Oracle;
address public immutable uniswapV2Factory;
address public immutable baseToken;

/// @dev Mapping from CNft address to underlying NFTX token address.
mapping(address => address) public underlyingNftxTokenAddress;

modifier onlyAdmin() {
    require(msg.sender == admin, "Sender must be the admin.");
    _;
}

---

Do address(0) check at changeAdmin function

The newAdmin variable passed in changeAdmin function does not have address(0) check on newAdmin.

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/CNftPriceOracle.sol#L55

function changeAdmin(address newAdmin) external onlyAdmin {
    admin = newAdmin;
}

If newAdmin = address(0) is set at the function, admin operations of CNftPriceOracle cannot be done at all.

It is worth adding address(0) check at changeAdmin function unless setting address(0) at newAdmin is expected behavior.

function changeAdmin(address newAdmin) external onlyAdmin {
    require(newAdmin != address(0));
    admin = newAdmin;
}

---

CErc20.sol/CToken.sol

Inputs validations at liquidateBorrowNftFresh function

liquidateBorrowNftFresh function has following input validations.

require(seizeIds.length == seizeAmounts.length, "SEIZE_IDS_SEIZE_AMOUNTS_MISMATCH");

It is worth adding several checks such as seizeIds.length != 0 or borrower != address(borrower) to have secure code.

require(seizeIds.length != 0, "...");
require(borrower != address(borrower), "...")

---

CNft.sol

No need to set 0 at uint256 variable

Following codes set 0 at uint256 variables.

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

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

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

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

uint256 totalAmount = 0;

vars.totalAmount = 0;

The default value of uint256 is 0, so there is no need to set 0. It can write like this to reduce gas cost.

uint256 totalAmount;

vars.totalAmount;

---

No need to define mintAllowedResult variable at mint function

mintAllowedResult variable is only used once.

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

uint mintAllowedResult = ComptrollerInterface(comptroller).mintAllowed(address(this), msg.sender, 0);
require(mintAllowedResult == 0, "CNFT: Mint is not allowed");

It can avoid defining mintAllowedResult to reduce the gas cost. It can write like this:

require(ComptrollerInterface(comptroller).mintAllowed(address(this), msg.sender, 0) == 0, "CNFT: Mint is not allowed");

---

No need to define siezeAllowedResult variable at seize function

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

uint siezeAllowedResult = ComptrollerInterface(comptroller).seizeAllowed(address(this), msg.sender, liquidator, borrower, 0);
require(siezeAllowedResult == 0, "CNFT: Seize is not allowed");

It can avoid defining siezeAllowedResult to reduce the gas cost. It can write like this:

require(ComptrollerInterface(comptroller).seizeAllowed(address(this), msg.sender, liquidator, borrower, 0) == 0, "CNFT: Seize is not allowed");

---

No need to define redeemAllowedResult variable at redeem function

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

uint redeemAllowedResult = ComptrollerInterface(comptroller).redeemAllowed(address(this), msg.sender, totalAmount);
require(redeemAllowedResult == 0, "CNFT: Redeem is not allowed");

It can avoid defining redeemAllowedResult to reduce the gas cost. It can write like this:

require(ComptrollerInterface(comptroller).redeemAllowed(address(this), msg.sender, totalAmount) == 0, "CNFT: Redeem is not allowed");

---

No need to define transferAllowedResult variable at safeBatchTransferFrom function

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

uint transferAllowedResult = ComptrollerInterface(comptroller).transferAllowed(address(this), from, to, vars.totalAmount);
require(transferAllowedResult == 0, "CNFT: Redeem is not allowed");

It can avoid defining transferAllowedResult to reduce the gas cost. It can write like this:

require(ComptrollerInterface(comptroller).transferAllowed(address(this), from, to, vars.totalAmount) == 0, "CNFT: Redeem is not allowed");

---

PriceOracleImplementation.sol

Use != 0 instead of > 0 to reduce gas cost

Following codebase uses > 0 on uint variables.

require(
    reserve1 > 0,
    "UniswapV2PriceOracle: Division by zero."
);

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/UniswapV2PriceOracle.sol#L26

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/UniswapV2PriceOracle.sol#L91

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/UniswapV2PriceOracle.sol#L115

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/UniswapV2PriceOracle.sol#L128

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/UniswapV2PriceOracle.sol#L149

The above mentioned codebase can be rewritten like this to reduce gas cost:

require(
    reserve1 != 0,
    "UniswapV2PriceOracle: Division by zero."
);

---

Setting 0 on uint256 variable is not necessary

The codebase sets 0 on uint256 variable.

uint256 numberUpdated = 0;
for (uint256 i = 0; i < pairs.length; ++i) {

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/UniswapV2PriceOracle.sol#L41-L42

It can be rewritten like this to reduce the gas cost.

uint256 numberUpdated;
for (uint256 i; i < pairs.length; ++i) {

---

Can use != 1 instead of > 1 at price1 function

This code uses > 1 but it can use != 1 instead.

require(length > 1, "UniswapV2PriceOracle: Only one observation.");

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/UniswapV2PriceOracle.sol#L152

length variable is uint256 so it cannot be less than 0. In addition, it has a condition: require(length > 0, "UniswapV2PriceOracle: No observations.");, so length variable should be more than or equal to 1. Therefore, the above code can be rewritten like this:

require(length != 1, "UniswapV2PriceOracle: Only one observation.");

---

CNftPriceOracle.sol

Use != 0 instead of > 0 at addAddressMapping function

cNfts.length will be uint so it can simply check != 0.

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/Oracles/CNftPriceOracle.sol#L63

require(
    cNfts.length > 0 && cNfts.length == nftxTokens.length,
    "CNftPriceOracle: `cNfts` and `nftxTokens` must have nonzero, equal lengths."
);

Rewriting the above code to use != 0 like this can reduce gas cost.

require(
    cNfts.length != 0 && cNfts.length == nftxTokens.length,
    "CNftPriceOracle: `cNfts` and `nftxTokens` must have nonzero, equal lengths."
);

---

No need to set 0 at uint256 variable

It sets 0 at uint256 i, but this is not needed.

for (uint256 i = 0; i < cNfts.length; ++i) {

Rewriting like this can reduce gas cost.

for (uint256 i; i < cNfts.length; ++i) {

---