r2moon
commented
3 years ago there is enough validation
Closed code423n4 closed 3 years ago
r2moon
commented
3 years ago there is enough validation
ghoul-sol
commented
3 years ago per sponsor comment, invalid
Handle
hyh
Vulnerability details
Impact
Asking to provide historic data proof doesn't imply that pricing is current, a malicious user can wait for market volatility and do deposit/borrow sequence with outdated price, borrowing more than current market value of supplied assets (for example, suppose asset is a semi-liquid token, whose value just decreased sharply; attacker can borrow against it more musd than it's current market value and leave the position for liquidation, which will not be feasible economically).
Request to provide historic data proof as a part of withdraw/borrow operations doesn't make system price current as providing outdated _data doesn't revert a transaction, the data is simply discarded.
Calling with outdated block number is possible as there are no conditions being placed for block number, it is simply retrieved and forwarded for saving in https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-cssr/contracts/adapter/UniswapV2TokenAdapter.sol#L87
If an attacker provide old piece of historic data proof, the check at https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-cssr/contracts/cssr/UniswapV2CSSR.sol#L87 just exit the update function, assuming that this piece of data is already updated. But it can be just a substantially old piece of data, which is indeed updated before, and this way withdraw/borrow goes on with no real update happened, in other words requesting a caller to provide the data doesn't guarantee that most current piece of historic data is indeed updated and pricing is current.
Proof of Concept
MochiVault.withdraw and borrow are public user facing functions, that use for price discovery engine.cssr().update(address(asset), _data) calls instead of engine.cssr().getPrice(address(asset)) used by MochiVault.liquidate and liquidatable functions. The update and getPrice calls differ in historic trading data saving, which happens in update, but is absent in getPrice. In other words it is required to send historic data proofs to UniswapV2CSSR.saveReserve function to be saved in observedData array and this data is used right away to obtain a price for withdraw/borrow.
Call sequence: MochiVault.withdraw and borrow -> engine.cssr().update(address(asset), _data) = MochiCSSRv0.update -> ICSSRAdapter.update = UniswapV2TokenAdapter.update -> uniswapCSSR.saveReserve or sushiCSSR.saveReserve = UniswapV2CSSR.saveReserve.
saveReserve updates observedData[blockNumber][pair] entry, which is then used IUniswapV2CSSR.getLiquidity and getExchangeRatio to determine current price (let's examine token's case, when ICSSRAdapter used is UniswapV2TokenAdapter): getPrice -> getPriceRaw -> getLiquidity, getExchangeRatio. The latter functions use historic data, that was updated with UniswapV2CSSR.saveReserve. https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-cssr/contracts/adapter/UniswapV2TokenAdapter.sol#L114
Recommended Mitigation Steps
Separate CSSR historic data update and user operations (the description below is duplicated as a gas saving measure) Now MochiVault.withdraw and borrow, which as public user facing functions, do historic CSSR data update as a part of asset price request. In the same time for both functions only current price is actually needed, which can be performed via getPrice, given that historic data is updated otherwise. https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/vault/MochiVault.sol#L189 https://github.com/code-423n4/2021-10-mochi/blob/main/projects/mochi-core/contracts/vault/MochiVault.sol#L219 Both float memory price = engine.cssr().update(address(asset), _data); calls can be replaced with float memory price = engine.cssr().getPrice(address(asset)); while 'bytes memory _data' to be removed from MochiVault.withdraw, borrow, increase, decrease function signatures. Data update to be called via UniswapV2CSSR.saveReserve directly via separate function via system script with necessary frequency. The need to update the data before getting a price is better served via separate system script from security standpoint as well as 1) _data can be for any block number, so malicious user can send old data as an update, UniswapV2CSSR.saveReserve doesn't throw being fed with old data, i.e. data update can effectively be void. 2) _data update is for one CSSR source only, i.e. uniswapCSSR or sushiCSSR, so even if valid recent data is being sent with withdraw/borrow for one of them, the other's data for getPrice can be still stalled.