DittoETH
Ditto
DeFiFoundryOracle
55,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

`Errors.InvalidTwapPrice()` is never invoked when `if (twapPriceInEther == 0)` is true

t0x1c

Summary & Vulnerability Details

The protocol expects to revert with Errors.InvalidTwapPrice() when twapPriceInEther == 0:

File: contracts/libraries/LibOracle.sol
​
85 uint256 twapPriceInEther = (twapPrice / Constants.DECIMAL_USDC) * 1 ether;
86 uint256 twapPriceInv = twapPriceInEther.inv();
87 if (twapPriceInEther == 0) {
88 revert Errors.InvalidTwapPrice(); // @audit : unreachable code
89 }

However, the control never reaches Line 88 when twapPriceInEther is zero. It rather reverts before that with error Division or modulo by 0.

NOTE: Due to this bug, Errors.InvalidTwapPrice() is never invoked/thrown by the protocol even under satisfactory conditions, even though it has been defined.

PoC

Since I could not find any helper function inside contracts/ or test/ which lets one set the twapPrice returned by uint256 twapPrice = IDiamond(payable(address(this))).estimateWETHInUSDC(Constants.UNISWAP_WETH_BASE_AMT, 30 minutes); to zero for testing purposes, I have created a simplified PoC which targets the problem area:

Save the following as a file named test/InvalidTwapPriceErrorCheck.t.sol and run the test via forge test --mt testInvalidTwapPriceErrNeverInvoked -vv. You will find that the test reverts with error Division or modulo by 0, but not with Errors.InvalidTwapPrice(). The PoC uses the same underlying math libraries and logic path as the protocol does in contracts/libraries/LibOracle.sol::baseOracleCircuitBreaker().

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.21;
​
import {Constants} from "contracts/libraries/Constants.sol";
import {Errors} from "contracts/libraries/Errors.sol";
import {U256} from "contracts/libraries/PRBMathHelper.sol";
import {OBFixture} from "test/utils/OBFixture.sol";
​
contract InvalidTwapPriceErrorCheck is OBFixture {
using U256 for uint256;
​
function getZeroTwapPriceInEther_IncorrectStyle_As_In_Existing_DittoProtocol()
internal
pure
returns (uint256 twapPriceInEther, uint256 twapPriceInv)
{
// fake the twapPrice to 0
uint256 twapPrice = 0; // IDiamond(payable(address(this))).estimateWETHInUSDC(Constants.UNISWAP_WETH_BASE_AMT, 30 minutes);
// Following code is copied as-is from
// `contracts/libraries/LibOracle.sol::baseOracleCircuitBreaker()#L85-L89`
twapPriceInEther = (twapPrice / Constants.DECIMAL_USDC) * 1 ether;
twapPriceInv = twapPriceInEther.inv();
if (twapPriceInEther == 0) {
revert Errors.InvalidTwapPrice(); // @audit : unreachable code
}
}
​
function getZeroTwapPriceInEther_CorrectStyle()
internal
pure
returns (uint256 twapPriceInEther, uint256 twapPriceInv)
{
// fake the twapPrice to 0
uint256 twapPrice = 0; // IDiamond(payable(address(this))).estimateWETHInUSDC(Constants.UNISWAP_WETH_BASE_AMT, 30 minutes);
twapPriceInEther = (twapPrice / Constants.DECIMAL_USDC) * 1 ether;
if (twapPriceInEther == 0) {
revert Errors.InvalidTwapPrice();
}
twapPriceInv = twapPriceInEther.inv();
}
​
function testInvalidTwapPriceErrNeverInvoked() public pure {
getZeroTwapPriceInEther_IncorrectStyle_As_In_Existing_DittoProtocol();
}
​
function testInvalidTwapPriceErrInvokedCorrectly() public {
vm.expectRevert(Errors.InvalidTwapPrice.selector);
getZeroTwapPriceInEther_CorrectStyle();
}
}

In the above test file, you can also run the test which invokes the "fixed" or "correct" code style via forge test --mt testInvalidTwapPriceErrInvokedCorrectly -vv. This will invoke the Errors.InvalidTwapPrice error, as expected.

Recommendations & Root Cause

The check on Line 87 (if condition) needs to be performed immediately after Line 85.

85 uint256 twapPriceInEther = (twapPrice / Constants.DECIMAL_USDC) * 1 ether;
+ 86 if (twapPriceInEther == 0) {
+ 87 revert Errors.InvalidTwapPrice();
+ 88 }
+ 89 uint256 twapPriceInv = twapPriceInEther.inv();
- 86 uint256 twapPriceInv = twapPriceInEther.inv();
- 87 if (twapPriceInEther == 0) {
- 88 revert Errors.InvalidTwapPrice();
- 89 }

The above fix needed to be done because the inv() call caused a revert even before control used to reach the if condition.

Impact

Protocol owner or developer monitoring for a revert due to Errors.InvalidTwapPrice() in the logs will never see it and will make debugging & issue resolution harder.

Tools Used

Manual audit & foundry.

Updates

Lead Judging Commences

0xnevi Lead Judge
about 2 years ago
0xnevi Lead Judge about 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-188

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.