M-02 [Oval] Attempts to Push Price to the CoinbaseOracle Will Always Fail #18

md0x · 2024-06-17T11:48:49Z

Fixes the following issue reported in the Oval Incremental Audit:

The CoinbaseOracle contract serves as an oracle for price data reported and signed by
Coinbase. In order to supply such price data to the contract, users have to call the
pushPrice function which, in case of the correct data passed, updates the price data for a
given asset. In order to verify the correctness of provided data, several checks are made,
including the check for a valid kind field. However, this check reverts when kind is different
than "price". The data signed by the Coinbase oracle will always contain kind equal to
"prices", resulting in the pushPrice function always reverting, making it impossible to
provide asset prices.
Consider verifying that the provided kind value is equal to "prices".

Changes:

CoinbaseOracle now takes a string memory _dataKind argument instead of having a hardcoded "price" value
Add forge tests that push real prices from Coinbase Oracle API
Small interface updates to simplify contract

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

linear · 2024-06-17T11:48:52Z

UMA-2647 M-02 [Oval] Attempts to Push Price to the CoinbaseOracle Will Always Fail

md0x · 2024-06-17T11:51:29Z

src/interfaces/coinbase/IAggregatorV3SourceCoinbase.sol

@@ -7,10 +7,10 @@ interface IAggregatorV3SourceCoinbase {
    function latestRoundData(string memory ticker)
        external
        view
-        returns (uint80 roundId, int256 answer, uint256 startedAt, uint256 updatedAt, uint80 answeredInRound);


Additionally, we can simplify the return values, given that they don’t match the Chainlink Aggregator interface anymore with the ticker argument.

md0x · 2024-06-17T11:52:12Z

src/oracles/CoinbaseOracle.sol

     * @param _reporter The address of the reporter allowed to push price data.
     */
-    constructor(uint8 _decimals, address _reporter) {
+    constructor(uint8 _decimals, string memory _dataKind, address _reporter) {


We now can pass a data kind in the constructor

if it's always prices then we should not yet the deployer to define this, I think? if it should always be a hard-coded value then we should hard-code it.

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

Reinis-FRP · 2024-06-18T08:02:28Z

src/oracles/CoinbaseOracle.sol

            uint256 timestamp, // e.g. 1629350000
            string memory ticker, // e.g. "BTC"
            uint256 price // 6 decimals
        ) = abi.decode(priceData, (string, uint256, string, uint256));

-        require(keccak256(abi.encodePacked(kind)) == keccak256(abi.encodePacked("price")), "Invalid kind.");
+        require(keccak256(abi.encodePacked(kind)) == keccak256(abi.encodePacked("prices")), "Invalid kind.");


it might be better to use constant for KIND

or bytes32 public immutable KIND_HASH = keccak256(abi.encodePacked("prices"));

Reinis-FRP · 2024-06-18T08:49:07Z

src/oracles/CoinbaseOracle.sol

+    function reporter() public view virtual returns (address) {
+        return 0xfCEAdAFab14d46e20144F48824d0C09B1a03F2BC;


better leave as immutable. If mock needs override then can do that in mock constructor

chrismaree · 2024-06-18T09:37:55Z

src/oracles/CoinbaseOracle.sol

@@ -8,48 +8,45 @@ import {IAggregatorV3SourceCoinbase} from "../interfaces/coinbase/IAggregatorV3S
 * @notice A smart contract that serves as an oracle for price data reported by a designated reporter.
 */
 contract CoinbaseOracle is IAggregatorV3SourceCoinbase {
-    address immutable reporter;
+    uint8 public immutable decimals = 6;


also if we make it immutable I think it should be all uppercase, no?

chrismaree

looks good so far but needs a bit of refinement to be more consistent. ever immutable should be uppercase, for example.

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

Reinis-FRP · 2024-06-18T10:02:28Z

src/oracles/CoinbaseOracle.sol

@@ -8,48 +8,47 @@ import {IAggregatorV3SourceCoinbase} from "../interfaces/coinbase/IAggregatorV3S
 * @notice A smart contract that serves as an oracle for price data reported by a designated reporter.
 */
 contract CoinbaseOracle is IAggregatorV3SourceCoinbase {
-    address immutable reporter;
+    address public reporter = 0xfCEAdAFab14d46e20144F48824d0C09B1a03F2BC;


this will have state reading overhead in production. better use the same approach as for decimals, like have upper case constant and lovercase getter function

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

feat: Fix Coinbase Oracle and test against production API

b2d5cd8

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

md0x commented Jun 17, 2024

View reviewed changes

md0x changed the title ~~feat: Fix Coinbase Oracle and test against production API~~ fix[oval-audit-m-02]: Attempts to Push Price to the CoinbaseOracle Will Always Fail Jun 17, 2024

md0x marked this pull request as ready for review June 17, 2024 12:57

md0x requested review from Reinis-FRP, chrismaree and daywiss June 17, 2024 12:57

md0x added 2 commits June 17, 2024 18:22

feat: hardcode values in coinbase adapter

6fc7258

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

refactor: move up reporter function

c2580bf

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

md0x changed the title ~~fix[oval-audit-m-02]: Attempts to Push Price to the CoinbaseOracle Will Always Fail~~ M-02 [Oval: Attempts to Push Price to the CoinbaseOracle Will Always Fail Jun 18, 2024

md0x changed the title ~~M-02 [Oval: Attempts to Push Price to the CoinbaseOracle Will Always Fail~~ M-02 [Oval] Attempts to Push Price to the CoinbaseOracle Will Always Fail Jun 18, 2024

Reinis-FRP reviewed Jun 18, 2024

View reviewed changes

chrismaree reviewed Jun 18, 2024

View reviewed changes

fix: comments

56b4279

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

Reinis-FRP reviewed Jun 18, 2024

View reviewed changes

feat: virtual function

b057756

Signed-off-by: Pablo Maldonado <pablo@umaproject.org>

md0x requested review from chrismaree and Reinis-FRP June 18, 2024 10:30

Reinis-FRP approved these changes Jun 18, 2024

View reviewed changes

md0x merged commit ff31b1b into master Jun 18, 2024
3 checks passed

md0x deleted the pablo/uma-2647-m-02-oval-attempts-to-push-price-to-the-coinbaseoracle-will branch June 18, 2024 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

M-02 [Oval] Attempts to Push Price to the CoinbaseOracle Will Always Fail #18

M-02 [Oval] Attempts to Push Price to the CoinbaseOracle Will Always Fail #18

md0x commented Jun 17, 2024 •

edited

Loading

linear bot commented Jun 17, 2024

md0x Jun 17, 2024

md0x Jun 17, 2024

chrismaree Jun 17, 2024

Reinis-FRP Jun 18, 2024

Reinis-FRP Jun 18, 2024 •

edited

Loading

Reinis-FRP Jun 18, 2024

chrismaree Jun 18, 2024

chrismaree left a comment

Reinis-FRP Jun 18, 2024

		function reporter() public view virtual returns (address) {
		return 0xfCEAdAFab14d46e20144F48824d0C09B1a03F2BC;

M-02 [Oval] Attempts to Push Price to the CoinbaseOracle Will Always Fail #18

M-02 [Oval] Attempts to Push Price to the CoinbaseOracle Will Always Fail #18

Conversation

md0x commented Jun 17, 2024 • edited Loading

linear bot commented Jun 17, 2024

md0x Jun 17, 2024

Choose a reason for hiding this comment

md0x Jun 17, 2024

Choose a reason for hiding this comment

chrismaree Jun 17, 2024

Choose a reason for hiding this comment

Reinis-FRP Jun 18, 2024

Choose a reason for hiding this comment

Reinis-FRP Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Reinis-FRP Jun 18, 2024

Choose a reason for hiding this comment

chrismaree Jun 18, 2024

Choose a reason for hiding this comment

chrismaree left a comment

Choose a reason for hiding this comment

Reinis-FRP Jun 18, 2024

Choose a reason for hiding this comment

md0x commented Jun 17, 2024 •

edited

Loading

Reinis-FRP Jun 18, 2024 •

edited

Loading