Re-org attack in factory `LiquidationPairFactory.sol`

[code423n4]

Lines of code

https://github.com/GenerationSoftware/pt-v5-cgda-liquidator/blob/7f95bcacd4a566c2becb98d55c1886cadbaa8897/src/LiquidationPairFactory.sol#L65-L108

Vulnerability details

Impact

Allowing creation of new LiquidationPairs by Re-org attack may adversely affect pricing in LiquidationPair.sol contracts.

Proof of Concept

The LiquidationPairFactory.sol#createPair() function deploys a new LiquidationPair using the create, where the address derivation depends only on the arguments passed. At the same time, some of the chains like Arbitrum and Polygon are suspicious of the reorg attack. LiquidationPairFactory.sol#createPair()

File: LiquidationPairFactory.sol
  function createPair(
    ILiquidationSource _source,
    address _tokenIn,
    address _tokenOut,
    uint32 _periodLength,
    uint32 _periodOffset,
    uint32 _targetFirstSaleTime,
    SD59x18 _decayConstant,
    uint112 _initialAmountIn,
    uint112 _initialAmountOut,
    uint256 _minimumAuctionAmount
  ) external returns (LiquidationPair) {
    LiquidationPair _liquidationPair = new LiquidationPair( // @audit-issue Reorg Attack
      _source,
      _tokenIn,
      _tokenOut,
      _periodLength,
      _periodOffset,
      _targetFirstSaleTime,
      _decayConstant,
      _initialAmountIn,
      _initialAmountOut,
      _minimumAuctionAmount
    );

    allPairs.push(_liquidationPair);
    deployedPairs[_liquidationPair] = true;

    emit PairCreated(
      _liquidationPair,
      _source,
      _tokenIn,
      _tokenOut,
      _periodLength,
      _periodOffset,
      _targetFirstSaleTime,
      _decayConstant,
      _initialAmountIn,
      _initialAmountOut,
      _minimumAuctionAmount
    );

    return _liquidationPair;
  }

Optimistic rollups (Optimism/Arbitrum) are also suspect to reorgs since if someone finds a fraud the blocks will be reverted, even though the user receives a confirmation. Can refer this an issue previously report here to have more understanding about it.

Tools Used

Manual review and previously report here

Recommended Mitigation Steps

Deploy createPair() via create2 with salt.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-sponsor]

asselstine marked the issue as sponsor confirmed

[HickupHH3]

Allowing this to remain as medium because it "adversely affect pricing". Would be nice to see more concrete proof though:

where the address derivation depends only on the arguments passed.

true for create2, not for create, so I suppose one could pass in less favourable arguments.

[c4-judge]

HickupHH3 marked the issue as selected for report

[stalinMacias]

Hey @HickupHH3 Can I ask how is the reorg-attack causing any harm to the protocol? The warden's statement " may adversely affect pricing in LiquidationPair.sol contracts." is very vague and there is not proof of how the pricing is impacted.

As for the previous report that the warden used as a reference for his finding, I think the reorg attack is valid in that protocol because the deployed contracts have a payable constructor that is expecting to receive ETH, and if the reorg attack happens then the ETH sent by the deployer is lost, but in this case, for this protocol, the deployed contracts are not payable, so what's the incentive to cause a "reorg-attack", and more important, what are the consequences?

IMHO, if there is no proof about how the pricing may be affected, this should be assessed as a Q/A

[HickupHH3]

@stalinMacias It's not so much causing a re-org attack, but taking advantage of it.

Because create is used, you could theoretically deploy with different params but have the same contract address in this scenario, then re-send (replay) the liquidator's tx to auction for more / less POOL tokens.

I shouldn't be doing the work to reason it out though, burden of proof lies with the wardens. None have sufficient justification to warrant the medium severity.

Downgrading to QA.

[c4-judge]

HickupHH3 changed the severity to QA (Quality Assurance)

[c4-judge]

HickupHH3 marked the issue as not selected for report

[c4-judge]

HickupHH3 marked the issue as grade-c

[Nadinnnn]

I agree with this issue being downgraded to QA, but still having an attack like you helped us understand more about how this issue can be exploited. The sponsor has also confirmed this issue and I think they will fix it. So I think this case can get grade A.

[c4-judge]

This previously downgraded issue has been upgraded by HickupHH3

[c4-judge]

HickupHH3 marked issue #31 as primary and marked this issue as a duplicate of 31

[c4-judge]

HickupHH3 marked the issue as satisfactory

[c4-judge]

HickupHH3 marked the issue as not a duplicate

[c4-judge]

HickupHH3 changed the severity to QA (Quality Assurance)

[c4-judge]

This previously downgraded issue has been upgraded by HickupHH3

[c4-judge]

This previously downgraded issue has been upgraded by HickupHH3

[c4-judge]

HickupHH3 marked the issue as duplicate of #31