Missing zero price check in `gas-price-oracle::update_price`.

[hats-bug-reporter[bot]]

Github username: @erictee2802 Twitter username: 0xEricTee Submission hash (on-chain): 0x4ec245fcc282db813f0fc961623809afa15d3b588fef34058bcb9b7b3f7e3f7c Severity: low

Description: Description

Missing zero price check in gas-price-oracle::update_price.

Attack Scenario

In gas-price-oracle::update_price:

 #[ink(message)]
        pub fn update_price(&mut self, new_price: u128) -> Result<(), OracleError> {
            self.ensure_owner()?;
            self.last_update = self.env().block_timestamp();
            self.last_price = new_price;
            self.env().emit_event(PriceUpdated {
                price: new_price,
                timestamp: self.last_update,
            });
            Ok(())
        }
// REDACTED BY ERICTEE

Noticed that there is lack of zero value check in new_price. As a result, the price can be set to zero accidentally.

Attachments

NA

1. Proof of Concept (PoC) File

Add the following test:

#[ink::test]
        fn set_zero_price() {
            let owner = default_accounts::<DefEnv>().alice;
            let init_price = 100;
            set_caller::<DefEnv>(owner);
            let mut oracle = Oracle::new(owner, init_price);
            let zero_price = 0;
            assert!(init_price != zero_price);
            oracle.update_price(zero_price).unwrap();
            assert_eq!(oracle.get_price(), (zero_price, 0));
        }

Cargo test Result:

running 5 tests
test oracle::tests::get_price_works_after_initialization ... ok
test oracle::tests::get_price_works_after_update ... ok
test oracle::tests::get_price_fails_on_non_owner ... ok
test oracle::tests::set_zero_price ... ok
test oracle::tests::set_owner_works ... ok

test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

2. Revised Code File (Optional) Consider making the following changes:

#[ink(message)]
        pub fn update_price(&mut self, new_price: u128) -> Result<(), OracleError> {
            self.ensure_owner()?;
++          if new_price == 0 {
++                return Err(OracleError::InvalidPrice);
++          }
            self.last_update = self.env().block_timestamp();
            self.last_price = new_price;
            self.env().emit_event(PriceUpdated {
                price: new_price,
                timestamp: self.last_update,
            });
            Ok(())
        }

[krzysztofziobro]

Don't see how that's a vulnerability, or how that check would help in any way

[0xEricTee]

It serves as an additional check to prevent 0 price from being used in the code logic.