code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/daae020331404647c661ab534d20093c875483e1/core/solidity/contracts/periphery/oracles/AnchoredViewRelay.sol#L74

Vulnerability details

Impact

AnchoredViewRelay is a mechanism that uses two independent oracles and checks whether the price from the main oracle does not deviate too much from the auxiliary oracle. An oracle using AnchoredViewRelay and the next two pairs can be added as the main and auxiliary oracle. The currentValue function is used to retrieve the current price, which is intended to provide always up-to-date prices. The basic OracleRelay implementation from which AnchoredViewRelay inherits also includes the peekValue function, which is widely used in the system and by definition does not have to, but may guarantee the current price. The problem will arise if the pair in AnchoredViewRelay is paired with an oracle that does not return the current price with peekValue but with currentValue (e.g. similar to CbEthEthOracle.sol). Although the price at the highest level (i.e. from AnchoredViewRelay) will be retrieved using currentValue, it retrieves prices from its oracles using peekValue, which may result in outdated prices.

Proof of Concept

File: core/solidity/contracts/periphery/oracles/OracleRelay.sol
23:   /// @dev Most oracles don't require a state change for pricing, for those who do, override this function
24:   function currentValue() external virtual returns (uint256 _currentValue) {
25:     _currentValue = peekValue();
26:   }

File: core/solidity/contracts/periphery/oracles/AnchoredViewRelay.sol
63:   /// @notice returns the price with 18 decimals without any state changes
64:   /// @dev some oracles require a state change to get the exact current price.
65:   ///      This is updated when calling other state changing functions that query the price
66:   /// @return _price the current price
67:   function peekValue() public view override returns (uint256 _price) {
68:     _price = _getLastSecond();
69:   }
70: 
71:   /// @notice Compares the main value (chainlink) to the anchor value (uniswap v3)
72:   /// @dev The two prices must closely match +-buffer, or it will revert
73:   /// @return _mainValue The current value of oracle
74:   function _getLastSecond() private view returns (uint256 _mainValue) {
75:     // get the main price
76:     _mainValue = mainRelay.peekValue();
77:     require(_mainValue > 0, 'invalid oracle value');
78: 
79:     uint256 _anchorPrice = anchorRelay.peekValue();
80:     require(_anchorPrice > 0, 'invalid anchor value');
81: 
82:     uint256 _buffer;
83:     if (IChainlinkOracleRelay(address(mainRelay)).isStale()) {
84:       /// If the price is stale the range percentage is smaller
85:       _buffer = (staleWidthNumerator * _anchorPrice) / staleWidthDenominator;
86:     } else {
87:       _buffer = (widthNumerator * _anchorPrice) / widthDenominator;
88:     }
89: 
90:     // create upper and lower bounds
91:     uint256 _upperBounds = _anchorPrice + _buffer;
92:     uint256 _lowerBounds = _anchorPrice - _buffer;
93: 
94:     // ensure the anchor price is within bounds
95:     require(_mainValue < _upperBounds, 'anchor too low');
96:     require(_mainValue > _lowerBounds, 'anchor too high');
97:   }

Tools Used

Manual review

Recommended Mitigation Steps

AnchoredViewRelay should have separate currentValue and peekValue implementations, and each should use the appropriate functions from the underlying oracles.

Assessed type

Oracle

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

minhquanym commented 1 year ago

To summary, in AnchoredViewRelay, both currentValue() and peekValue() will call to relay.peekValue()

c4-sponsor commented 1 year ago

0xShaito marked the issue as disagree with severity

0xShaito commented 1 year ago

The implementations used in anchoredViewOracle do not need state changes and their prices are readily available at any time and correct as it uses chainlink and uniswap.

currentValue is meant as a fix for other types of oracles that actually need to update state like curve pools to avoid manipulations.

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)