bunker.finance contest - sorrynotsorry's results

Lines of code

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

Vulnerability details

Impact

Use of deprecated Chainlink function latestAnswer According to Chainlink's documentation, the latestAnswer function is deprecated. This function does not error if no answer has been reached but returns 0, causing an incorrect price feed to USDC Price.

Proof of Concept

int256 usdcPrice = ChainlinkFeed(0x986b5E1e1755e3C2440e960477f25201B0a8bbD4).latestAnswer();

https://github.com/bunkerfinance/bunker-protocol/blob/752126094691e7457d08fc62a6a5006df59bd2fe/contracts/PriceOracleImplementation.sol#L29 ChainLink Data Feeds API Reference

Tools Used

Manual Review

Recommended Mitigation Steps

Use the latestRoundData function to get the price instead. Add checks on the return data with proper revert messages if the price is stale or the round is uncomplete, as example code below;

(uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound) = oracle.latestRoundData();
require(answeredInRound >= roundID, "...");
require(timeStamp != 0, "...");

Judge has assessed an item in Issue #94 as Medium risk. The relevant finding follows:

QA (LOW / NON-CRITICAL)

• The scoped contracts use approve() method which is prone to attack vectors as described here A potential fix includes preventing a call to approve if all the previous tokens are not spent through adding a check that the allowed balance is 0: require(allowed[msg.sender][_spender] == 0)

• The whole project has different solidity compiler ranges ^0.5.16 to ^0.8.0 referenced. This leads to potential security flaws between deployed contracts depending on the compiler version chosen for any particular file. It also greatly increases the cost of maintenance as different compiler versions have different semantics and behavior.

• Floating Pragma used in CEther.sol, CNft.sol, ERC1155Enumerable.sol ,CErc20.sol , CToken.sol , Comptroller.sol , CNftPriceOracle.sol , UniswapV2PriceOracle.sol. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma (i.e. by not using ^) helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively. Reference

• The function requireNoError() of CEther.sol contains 2 checks on errCode == uint(Error.NO_ERROR). After the first check it returns. After this errCode == uint(Error.NO_ERROR) will never be true, so doesn't have to be checked. Team might consider to replace require(errCode == uint(Error.NO_ERROR), string(fullMessage)); with require(false, string(fullMessage));

• transfer method is used inside the codebase. Since these methods use 2300 gas stipend which is not adjustable,it may likely to get broken when calling a contract's fallback function if any contract participates in the auction. Reference Link -1, Reference Link -2

• The project files are having Solidity compiler version is 0.5.16 which is outdated. Currently the compiler version is v0.8.13.

• At Comptroller.sol#L346, in order to reach the full cap, the logic should be;

require(nextTotalBorrows <= borrowCap, "market borrow cap reached");

instead of

require(nextTotalBorrows < borrowCap, "market borrow cap reached");

• The contract require statements are based on NO_ERROR is equivalent to 0 condition. However in most locations there is an explicit check for NO_ERROR and comparing with 0 allows for possible future mistakes (especially if the enums would change in ErrorReporter.sol) The team might consider to replace 0 with the appropriate version of ...NO_ERROR References: CToken.sol: function transferTokens(...if (allowed != 0) { function mintFresh(...if (allowed != 0) { function redeemFresh(...if (allowed != 0) { function borrowFresh(...if (allowed != 0) { function repayBorrowFresh(...if (allowed != 0) { function liquidateBorrowFresh(...if (allowed != 0) { function seizeInternal(...if (allowed != 0) { Comptroller.sol: function exitMarket(...if (allowed != 0) {...require(oErr == 0, "exitMarket: getAccountSnapshot failed"); // semi-opaque error code function getHypotheticalAccountLiquidityInternal(...if (oErr != 0) { // semi-opaque error code, we assume NO_ERROR == 0 is invariant between upgrades

• requireNoError() of CEther.sol is used to check for errors. This is used most of the time, however it isn't used in below;

function getCashPrior() internal view returns (uint) {
    (MathError err, uint startingBalance) = subUInt(address(this).balance, msg.value);
    require(err == MathError.NO_ERROR);
    return startingBalance;
}

The team might consider to replace require(err == MathError.NO_ERROR); with: requireNoError(err, "getCashPrior failed");

• There are expressions in CToken.sol like uint(-1) for max. values. However, the team can consider to implicitly state the infinite allowance by using type(uint256).max instead of uint(-1)

• ComptrollerInterface is declared with contract keyword. However it should be declared with interface keyword.

• At CNFt.sol initialize function, there is no address(0) check for _comptroller no zero bytes check for _uri,

• At ERC1155Enumerable.sol, there is no zero value check for _uri in __ERC1155Enumerable_init function.

• The codebase is having lack of NatSpec comments such as @notice @dev @param on many places. It's recommended to fully annote the contract by using NatSpec. Reference

• There is no address(0) check at PriceOracleImplementation.sol constructor function.

• There is no address(0) check at CNftPriceOracle.sol constructor function for _admin, _uniswapV2Oracle _uniswapV2Factory , _baseToken , newAdmin for changeAdmin() function.

• At CNftPriceOracle.sol, addAddressMapping() function, an expensive loop used by using SSTORE's. Incrementing state_variable in a loop incurs a lot of gas because of expensive SSTOREs, which might lead to an out-of-gas and halting the process.

        for (uint256 i = 0; i < cNfts.length; ++i) {
            address underlying = cNfts[i].underlying();
            require(
                underlyingNftxTokenAddress[underlying] == address(0),
                "CNftPriceOracle: Cannot overwrite existing address mappings."
            );
            underlyingNftxTokenAddress[underlying] = nftxTokens[i];
        }