code-423n4 / 2023-10-wildcat-findings

14 stars 10 forks source link

`create2` return value is not checked and it does not revert if used in assembly #623

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/libraries/LibStoredInitCode.sol#L115 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketController.sol#L354 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatMarketControllerFactory.sol#L297

Vulnerability details

Impact

deployMarket() in WildcatMarketController.sol and deployController() in WildcatMarketControllerFactory.sol has used the create2 opcode which uses the salt and in turn allows the new contract to be deployed at a consistent, deterministic address. These function does not revert properly if there is a failed deployment or revert from the create2 opcode as it does not check the returned address for bytecode. The create2 opcode returns the expected address which will never be the zero address, However, raw create2 has been used in assembly therefore it does not check the return value by default unlike used in solidity context.

Proof of Concept

deployMarket() in WildcatMarketController.sol and deployController() in WildcatMarketControllerFactory.sol under the hood has used create2WithStoredInitCode() from LibStoredInitCode.sol.

    LibStoredInitCode.create2WithStoredInitCode(marketInitCodeStorage, salt);

and

    LibStoredInitCode.create2WithStoredInitCode(controllerInitCodeStorage, salt);

create2WithStoredInitCode() from LibStoredInitCode.sol has shown as below,

  function create2WithStoredInitCode(
    address initCodeStorage,
    bytes32 salt,
    uint256 value
  ) internal returns (address deployment) {
    assembly {
      let initCodePointer := mload(0x40)
      let initCodeSize := sub(extcodesize(initCodeStorage), 1)
      extcodecopy(initCodeStorage, initCodePointer, 1, initCodeSize)
>>    deployment := create2(value, initCodePointer, initCodeSize, salt)
    }
  }

As it can be seen it does not check the return value. Zero address check must be added to revert for create2 if specifically used in assembly

The CREATE2 opcode does not revert the transaction. Check the evm.codes

To understand, how it differs from using in solidity context to assembly context, Please refer this comment where the issue is explained with poc and remixIDE snapshot.

Tools Used

Manual review

Recommended Mitigation Steps

Check the return value of create2.

For example:


// some create2 code

iszero(extcodesize(deployment)){

  // some revert condition
  .  .  .
}

Assessed type

Other

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #28

c4-judge commented 1 year ago

MarioPoneder marked the issue as not a duplicate

c4-judge commented 1 year ago

MarioPoneder changed the severity to QA (Quality Assurance)

MarioPoneder commented 1 year ago

Insufficent discussion of impact compared to former duplicates

c4-judge commented 1 year ago

MarioPoneder marked the issue as grade-c