code-423n4 / 2023-08-pooltogether-findings

4 stars 3 forks source link

The user can front-run the liquidation pair in VaultBooster #27

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L193 https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L211

Vulnerability details

Impact

In VaultBooster.sol, the user can set the liquidationPair address inside of a setBoost() function. In the case if the liquidationPair was created before and the user just points out to the existing liquidation pair, there is a possibility of front-run.

Proof of Concept

The liquidatior will call swapExactAmountOut() in LiquidationRouter to start the process of liquidation and eventually liquidate() inside of a VaultBooster will be called. The problem is that the user can front-run the liquidator and call withdraw() before that. In this situation, the amountAvailable for the liquidator becomes 0 and his transaction is reverted resulting in losing money on gas costs (_amountOut > amountAvailable check will fail):

https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L188-196 https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L211-236

The problem is possible because the VaultBooster relies on balanceOf(address(this)) to compute the available amount that can be easily manipulated:

https://github.com/GenerationSoftware/pt-v5-vault-boost/blob/9d640051ab61a0fdbcc9500814b7f8242db9aec2/src/VaultBooster.sol#L190

Tools Used

Manual review.

Recommended Mitigation Steps

Change the logic so that the user can front-run the liquidator by withdrawing the funds, possibly some additional fees should be introduced.

Assessed type

MEV

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #69

c4-pre-sort commented 1 year ago

raymondfam marked the issue as not a duplicate

c4-pre-sort commented 1 year ago

raymondfam marked the issue as primary issue

asselstine commented 1 year ago

The intention of the Vault Booster contract is to allow a user to deposit tokens to contribute to a Vault. Why would they troll the liquidators? If they don't want the liquidators to run they should not have created the Vault Booster contract and deposited tokens in the first place.

c4-sponsor commented 1 year ago

asselstine marked the issue as disagree with severity

c4-sponsor commented 1 year ago

asselstine marked the issue as sponsor disputed

HickupHH3 commented 1 year ago

Agree with sponsor, in this context there's nothing to be gained by the user frontrunning.

c4-judge commented 1 year ago

HickupHH3 marked the issue as unsatisfactory: Invalid