search

code-423n4 / 2022-05-backd-findings

0 stars 0 forks source link

FeeBurner initiates swap without any slippage checks if Chainlink oracle fails #44

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/tokenomics/FeeBurner.sol#L43-L88 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/swappers/SwapperRouter.sol#L414-L425 https://github.com/code-423n4/2022-05-backd/blob/main/protocol/contracts/swappers/SwapperRouter.sol#L439

Vulnerability details

Impact

While the SwapperRouter contract isn't explicitly in scope, it's a dependency of the FeeBurner contract which is in scope. So I think it's valid to make this submission.

The SwapperRouter contract uses the chainlink oracle to compute the minimum amount of tokens it should expect from the swap. The value is then used for the slippage check. But, if the chainlink oracle fails, for whatever reason, the contract uses 0 for the slippage check instead. Thus there's a scenario where swaps initiated by the FeeBurner contract can be sandwiched.

Proof of Concept

  1. multiple swaps initiated through FeeBurner.burnToTarget()
  2. SwapperRouter calls _minTokenAmountOut() to determine min_out parameter.
  3. minTokenAmountOut() returns 0 when Chainlink oracle fails

Tools Used

none

Recommended Mitigation Steps

Either revert the transaction or initiate the transaction with a default slippage of 99%. In the case of Curve, you can get the expected amount through get_dy() and then multiply the value by 0.99. Use that as the min_out value and you don't have to worry about chainlink

chase-manning commented 2 years ago

This is intended functionality. If there is no oracle for a token, we still want to swap it, even if this presents a possible sandwich attack. It should be rare for a token to not have an oracle, and when it does we would rather accept slippage as opposed to not being able to swap it at all.

GalloDaSballo commented 2 years ago

I acknowledge the sponsor reply that they want to offer a service to the end user in allowing any swappable token to be used.

While I believe the intent of the sponsor is respectable, the reality of the code is that it indeed allows for price manipulation and extraction of value, personally I would recommend end users to perform their own swaps to ensure a more reliable outcome.

That said, because the code can be subject to leak of value, I believe Medium Severity to be appropriate