Hubble contest - ye0lde's results

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L106-L122 https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L134

Vulnerability details

Impact

The getLatestRoundData function in the contract Oracle.sol fetches the latestPrice directly from a Chainlink aggregator using the latestRoundData function. While latestPrice is checked for < 0 and staleness, there is no check if the value is != 0. This could lead to incorrect prices for the user and general instability.

Proof of Concept

https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L106-L122

    function getLatestRoundData(AggregatorV3Interface _aggregator)
        internal
        view
        returns (
            uint80,
            uint256 finalPrice,
            uint256
        )
    {
        (uint80 round, int256 latestPrice, , uint256 latestTimestamp, ) = _aggregator.latestRoundData();
        finalPrice = uint256(latestPrice);
        if (latestPrice < 0) {
            requireEnoughHistory(round);
            (round, finalPrice, latestTimestamp) = getRoundData(_aggregator, round - 1);
        }
        return (round, finalPrice, latestTimestamp);
    }

getRoundData has a similar problem here: https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L134

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

I recommend making the following change:

     function getLatestRoundData(AggregatorV3Interface _aggregator)
        internal
        view
        returns (
            uint80,
            uint256 finalPrice,
            uint256
        )
    {
        (uint80 round, int256 latestPrice, , uint256 latestTimestamp, ) = _aggregator.latestRoundData();
        finalPrice = uint256(latestPrice);
        if (latestPrice <= 0) {     // @audit check for zero
            requireEnoughHistory(round);
            (round, finalPrice, latestTimestamp) = getRoundData(_aggregator, round - 1);
        }
        return (round, finalPrice, latestTimestamp);
    }

And the fix for getRoundData at l#134 is similar: while (latestPrice <= 0) {

Hubble QA Report

Unless otherwise noted, manual auditing and testing were done using Visual Studio Code and Remix. The audit was done from February 17-23, 2022 by ye0lde through code4rena.

Overall, I found the code to be clear to follow and read. I'd recommend the team improve the supporting documentation to give a better overall understanding of the protocol.

• Findings
  • L-1 - No validation of parameter price in setStablePrice (Oracle.sol)
  • L-2 - Unsafe type cast in getTwapPrice (AMM.sol)
  • NC-1 - Typo in getUnderlyingTwapPrice (Oracle.sol)

Findings

L-1 - No validation of parameter price in setStablePrice (Oracle.sol)

Impact

The setStablePrice function does not do any validation of the price parameter before setting stablePrice[underlying] = price. While this is a governance function, once stablePrice[underlying] is set to any non-zero value this overrides any aggregator calls made to access the current price by function getUnderlyingPrice and getUnderlyingTwapPrice. underlying is checked but not the price itself.

Proof of Concept

setStablePrice is here: https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L169-L172

function setStablePrice(address underlying, int256 price) external onlyGovernance {
    requireNonEmptyAddress(underlying);
    stablePrice[underlying] = price;
}

Recommended Mitigation Steps

Consider adding a check for price > 0

---

L-2 - Unsafe type cast in getTwapPrice (AMM.sol)

Impact

getTwapPrice performs an unsafe type cast to uint128 without checking if the value actually fits into 128 bits. This typecast doesn't seem to directly lead to an exploit but safe typecasts should still be implemented for additional security.

Proof of Concept

The type cast is here: https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/AMM.sol#L392

Recommended Mitigation Steps

I suggest the following change:

function getTwapPrice(uint256 _intervalInSeconds) public view returns (int256) {
    return (_calcTwap(_intervalInSeconds).toInt256()); 
}

---

NC-1 - Typo in getUnderlyingTwapPrice (Oracle.sol)

Impact

Code clarity

Proof of Concept

The typo is here: https://github.com/code-423n4/2022-02-hubble/blob/ed1d885d5dbc2eae24e43c3ecbf291a0f5a52765/contracts/Oracle.sol#L53

Recommended Mitigation Steps

Change form to from.

---