Open c4-submissions opened 1 year ago
Should we not be prioritising liveness here over validating chainlink results?
0xleastwood marked the issue as primary issue
It seems important to avoid using stale price data which can be readily arbitraged. Severity seems correct.
0xleastwood marked the issue as selected for report
elmutt (sponsor) confirmed
Lines of code
https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L32
Vulnerability details
Summary
The current price implementation for the VotiumStrategy token uses a potentially invalid Chainlink response. This price is then used to calculate the price of AfEth and, subsequently, the amount of tokens to mint while depositing.
Impact
The price of VotiumStrategy tokens are determined by taking the amount of deposited CVX in the strategy, and multiplied by the current price of CVX in terms of ETH. This price is fetched using Chainlink in the
ethPerCvx()
function:https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategyCore.sol#L156-L186
As we can see from the previous snippet of code, if the
_validate
flag is off, then no validation is done, it can even return an uninitialized response from a failed call given the usage of thetry/catch
structure. This means that it can invalid price, stale price, or even zero when the call fails.The VotiumStrategy
price()
function callsethPerCvx(false)
, which means it carries forward any invalid CVX/ETH price.https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/strategies/votium/VotiumStrategy.sol#L31-L33
The price of VotiumStrategy is then used in the AfEth contract to calculate its price and determine the amount of tokens to mint in
deposit()
https://github.com/code-423n4/2023-09-asymmetry/blob/main/contracts/AfEth.sol#L133-L169
The VotiumStrategy price is first used in line 138 to calculate its TVL (
vEthValueInEth
). Any invalid price here will also mean an invalid price for AfEth.Then both the AfEth price (line 151) and again the VotiumStrategy price (line 164) are used in
deposit()
to calculate the number of minted tokens. Depending on the direction of the wrong price, this means that the user will be minted more or less tokens than it should.Proof of Concept
Let's suppose the Chainlink feed is stale and the current price of CVX/ETH has increased since then.
deposit()
to create a new position in AfEth.priceBeforeDeposit
) in order to know how many tokens should be minted.price()
implementation will calculate the Votium strategy TVL usingethPerCvx(false)
, which will successfully return the stale price.priceBeforeDeposit
, since this price is lower than the expected "real" price the user will be minted more tokens than expected.Recommendation
Change the
ethPerCvx()
argument totrue
to make sure prices coming from Chainlink are correctly validated.Assessed type
Oracle