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

4 stars 3 forks source link

Attacker can frontrun createPair() to deploy at the same address #71

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

_liquidationPair are created from the factory via CREATE1, an attacker can frontrun createPair() to deploy at the same address but with different config. If the deployed chain reorg, a different vault might also be deployed at the same address.

File: src/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(
      _source,
      _tokenIn,
      _tokenOut,
      _periodLength,
      _periodOffset,
      _targetFirstSaleTime,
      _decayConstant,
      _initialAmountIn,
      _initialAmountOut,
      _minimumAuctionAmount
    );

   // Some code

  }

Proof of Concept

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

1) Charlie setup a bot to monitor the mempool when Bob deploy a new _liquidationPair 2) Charlie's bot saw a deployment by Bob at 0x1234, fire a tx to deposit immediately 3) Alice frontrun Bob's deployment by deploying a malicious _liquidationPair at 0x1234 4) Charlie's transaction ended up deposited into Alice's malicious vault

Tools Used

Manual Review

Recommended Mitigation Steps

Use CREATE2 for such contracts with msg.sender containing salt.

Assessed type

Other

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #52

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 duplicate of #169

c4-judge commented 1 year ago

HickupHH3 marked the issue as satisfactory

c4-judge commented 1 year ago

HickupHH3 changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

HickupHH3 marked the issue as grade-c

c4-judge commented 1 year ago

This previously downgraded issue has been upgraded by HickupHH3

thebrittfactor commented 1 year ago

For transparency, the judge requested to remove the unsatisfactory label.