Front-running of liquidations can cause stealing of prize tokens

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L570 https://github.com/GenerationSoftware/pt-v5-prize-pool/blob/4bc8a12b857856828c018510b5500d722b79ca3a/src/PrizePool.sol#L312

Vulnerability details

Impact

A liquidation of yield can be front run to steal the prize tokens deposited by the victim and to contribute them to the prize pool on behalf of the attacker. The attacker will receive vault shares in exchange for victim's prize tokens; the victim will receive nothing and will lose their prize tokens.

Proof of Concept

When liquidating yield via the Vault.liquidate function, the caller is expected to contribute some amount of prize tokens to the prize pool: the _prizePool.contributePrizeTokens function is called to perform the contribution. However, neither the Vault contract nor the PrizePool contract pool the prize tokens from the caller. The PrizePool.contributePrizeTokens function expects that prize tokens have already been deposited to the contract (PrizePool.sol#L312-L315):

/// @notice Contributes prize tokens on behalf of the given vault. The tokens should have already been transferred to the prize pool.

Since the Vault.liquidate function doesn't pull prize tokens from the caller, the only way for the caller to deposit the tokens is to transfer them to the prize pool contract in a separate transaction, before calling the liquidate function. This opens up the opportunity to front run the liquidate call: a MEV bot cal intercept the liquidate call and replicate it to liquidate yield using the prize tokens deposited by the victim. The front runner will receive vault shares while providing no prize tokens; the victim will lose their prize tokens and won't receive any shares.

Tools Used

Manual review

Recommended Mitigation Steps

In the Vault.liquidate function, consider pulling prize tokens from the caller (or rather the _account address) by using the ERC20's safeTransferFrom function and transfer them to the prize pool.

Assessed type

Token-Transfer

[Picodes]

I assume the liquidation contract is assumed to send the tokens to the contract beforehand in the same tx

[c4-judge]

Picodes marked the issue as primary issue

[asselstine]

This is a design choice; we wanted to allow liquidity to come from anywhere and not require the caller to hold liquidity. This means that there need be only one token transfer. The two-stage process is only intended for the Liquidator smart contract, not the end user.

The liquidator was not within scope, but you can see how we handle it in the swapExactAmountOut call. In this call there are only two transfers: from the liquidator to the prize pool, and from the vault to the liquidators. It's very efficient.

[c4-sponsor]

asselstine marked the issue as sponsor disputed

[asselstine]

Note: this is also how a Uniswap V2 and V3 Pair makes multiple swaps between pairs efficient; by only doing a transfer out and asserting the transfer-in balance.

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid