search

code-423n4 / 2022-06-badger-findings

0 stars 0 forks source link

Yield can be lost due to not specifying `limit` when transferring `auraBAL` to `BAL/ETH BPT` #122

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L249

Vulnerability details

Impact

In _harvest(), when swapping auraBAL to BAL/ETH BPT the limit variable which specifies the minimum amount of tokens that are to be received (when singleSwap.kind=GIVEN_IN) is set to 0. This means that when the swap is made, the transaction can be frontrun and lose a large portion of yield due to slippage.

swap(SingleSwap singleSwap,
     FundManagement funds,
     uint256 limit,
     uint256 deadline) returns (uint256 amountCalculated[In/Out])

I believe this a high severity issue because this can cause a large substantial loss of yield thereby rendering the whole purpose of the entire strategy meaningless.

Proof of Concept

  1. Contract tries to swap auraBAL to BAL/ETH BPT and transaction is added to mempool
  2. An attacker sees the transaction on the mempool, frontruns transaction and swaps a large amount of BAL/ETH BPT to auraBAL driving the price of BAL/ETH BPT up
  3. The contracts make an unfavourable swap for a much higher price leading to less tokens received, as limit is 0, the transaction goes through.
  4. Attacker sells auraBAL and makes profit at expense of the contract

Tools Used

VS Code

Recommended Mitigation Steps

Specify a minimum for limit using queryBatchSwap from the BalancerHelpers contract. reference

GalloDaSballo commented 2 years ago

Dup of #155 Disagree with High Also note that recommended mitigation is ineffective per the BAL docs