bunker.finance contest - throttle's results

Lines of code

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

Vulnerability details

Impact

Deprecated Chainlink oracle API. API might stop working. Prices could be outdated. Protocol might need to be redeployed or false prices might lead to users losing funds.

Proof of Concept

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

The contracts use Chainlink’s deprecated API latestAnswer(). Such functions might suddenly stop working if Chainlink stopped supporting deprecated APIs.

Additionally, one cannot check if the returned price is fresh. The price might by stale (old historical price).

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, for example:

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

[L-01] No event emission for new nft price oracle

Description

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

Recommended Mitigation Steps

Consider emitting an event

-----------------------------------------------------------------

[L-02] Missing 0 address check

Description

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

New admin is not checked against 0 value.

Recommended Mitigation Steps

Consider checking if new admin address is not 0

-----------------------------------------------------------------

[G-01] Function redeemAllowed() could be simplified.

Description

Function redeemAllowed() could be simplified.

function redeemAllowed(address cAsset, address redeemer, uint redeemTokens) external returns (uint) {
    uint allowed = redeemAllowedInternal(cAsset, redeemer, redeemTokens);
    if (allowed != uint(Error.NO_ERROR)) {
        return allowed;
    }

    // Keep the flywheel moving
    // updateCompSupplyIndex(cAsset);
    // distributeSupplierComp(cAsset, redeemer);

    return uint(Error.NO_ERROR);
}

could be rewritten to

function redeemAllowed(address cAsset, address redeemer, uint redeemTokens) external returns (uint) {
    return redeemAllowedInternal(cAsset, redeemer, redeemTokens);
}

Recommended Mitigation Steps

Consider rewriting

-----------------------------------------------------------------

[G-02] Cheaper for loop

Description

This:

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

can be optimized to

for (uint256 i; i < len;) {
    ...
    unchecked { ++i };
}

Recommended Mitigation Steps

Consider rewriting

-----------------------------------------------------------------