bunker.finance contest - GimelSec's results

Lines of code

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

Vulnerability details

Impact

latestAnswer function is deprecated, which doesn’t return an error but returns 0, and It is not able to check if the price is stale.

Proof of Concept

In the getUnderlyingPrice function of PriceOracleImplementation.sol, it uses ChainlinkFeed(0x986b5E1e1755e3C2440e960477f25201B0a8bbD4).latestAnswer() which will return the last value, but it doesn’t check if the value is fresh or not.

Tools Used

vim

Recommended Mitigation Steps

Use latestRoundData rather than latestAnswer and check the value:

     (
        roundId,
        answer,
        startedAt,
        updatedAt,
        answeredInRound
      ) = aggregator.latestRoundData();
      require(answer > 0, "Invalid answer");
      require(updatedAt != 0, "Invalid updatedAt");
      require(answeredInRound >= roundId, "Stale price");

Summary

We list 2 low-critical findings:

• (Low) Deploy CNft as an upgradable contract by proxy
• (Low) floating pragma

(Low) Deploy CNft as an upgradable contract by proxy

Impact

In CNftTest.ts, it deploys CNft without using a proxy contract. Use a proxy contract to deploy CNft for an upgradable contract, or it’s not necessary to use @openzeppelin/contracts-upgradeable.

Proof of Concept

CNft.sol uses ReentrancyGuardUpgradeable and OwnableUpgradeable: https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/CNft.sol#L16

Tools Used

vim

Recommended Mitigation Steps

Use proxy to deploy: https://docs.openzeppelin.com/upgrades-plugins/1.x/hardhat-upgrades Or there’s no need to use upgradable contracts ReentrancyGuardUpgradeable and OwnableUpgradeable, use ReentrancyGuard and Ownable instead.

(Low) floating pragma

Impact

Floating pragma may cause unexpected compilation time behaviour and introduce unintended bugs.

Proof of Concept

Oracles/CNftPriceOracle.sol
2:pragma solidity ^0.8.0;

Oracles/UniswapV2PriceOracle.sol
2:pragma solidity ^0.8.0;

CNft.sol
2:pragma solidity ^0.8.0;

Tools Used

vim

Recommended Mitigation Steps

Don't use ^, lock pragma to ensure compiler version. e.g. pragma solidity 0.8.0;

Save gas in for loops by unchecked arithmetic

The for loop has no overflow risk of i. Use an unchecked block to save gas.

Proof of Concept

Oracles/CNftPriceOracle.sol
66:        for (uint256 i = 0; i < cNfts.length; ++i) {

Oracles/UniswapV2PriceOracle.sol
42:        for (uint256 i = 0; i < pairs.length; ++i) {

Comptroller.sol
119:        for (uint i = 0; i < len; i++) {
199:        for (uint i = 0; i < len; i++) {
591:        for (uint i = 0; i < assets.length; i++) {
928:        for (uint i = 0; i < allMarkets.length; i ++) {
1223:        for (uint i = 0; i < cTokens.length; i++) {
1229:                for (uint j = 0; j < holders.length; j++) {
1235:                for (uint j = 0; j < holders.length; j++) {
1240:        for (uint j = 0; j < holders.length; j++) {

CNft.sol
50:        for (uint256 i; i < length; ++i) {
62:                for (uint256 i; i < length; ++i) {
72:                for (uint256 i; i < length; ++i) {
98:        for (uint256 i; i < length; ++i) {
122:        for (uint256 i; i < length; ++i) {
145:                for (uint256 i; i < length; ++i) {
151:                for (uint256 i; i < length; ++i) {
176:        for (uint256 i; i < vars.length; ++i) {

ERC1155Enumerable.sol
51:        for (uint256 i; i < ids.length; ++i) {

Recommendation

Use unchecked blocks to avoid overflow checks, or use ++i rather than i++ if you don't use unchecked blocks.

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