DoS: Attacker May Front-Run CoreFactory.createProject() Or CoreFactory.addCollection() With A collection.id Causing Future Transactions With The Same collection.id to Revert

[code423n4]

Lines of code

https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreFactory.sol#L147-L151
https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreFactory.sol#L164
https://github.com/code-423n4/2022-03-joyn/blob/c9297ccd925ebb2c44dbc6eaa3effd8db5d2368a/core-contracts/contracts/CoreFactory.sol#L50-L56

Vulnerability details

Impact

A _collection.id may only be used once in CoreFactory._createCollection() since the the contract is deployed using the create2 opcode with a repeated salt and contract bytecode will fail to deploy a contract. Furthermore, the modifier onlyAvailableCollection will fail if a collection has already been made with that ID.

The result is an attacker may front-run any createProject() or addCollection() transaction in the mem pool and create another createProject() or addCollection() transaction with a higher gas price that uses the same _collection.id but changes the other fields such as the msg.sender which will make them the owner of the contract that is deployed. The original transaction will revert and the user will not be able to send any more transaction with this _collection.id.

The user would therefore have to generate a new _collection.id. However, the attack is repeateable and there is no guarantee this new ID will be used to create a project successfully without the attacker front-running the transaction again.

Proof of Concept

collections[_collection.id] is set in _createCollection()

    collections[_collection.id] = coreCollection;

It also has the onlyAvailableCollection which will cause future transactions to revert.

  modifier onlyAvailableCollection(string memory _collectionId) {
    require(
      collections[_collectionId] == address(0),
      'CoreFactory: Unavailable collection id'
    );
    _;
  }

The use of salt will cause an repeated _collection.id to fail.

    address coreCollection = address(
      new CoreProxy{salt: keccak256(abi.encodePacked(_collection.id))}(
        collection
      )
    );

Recommended Mitigation Steps

One possible mitigation of this issue is to have the collection.id be a hash including the msg.sender and a salt where the salt is a parameter to the function.

bytes32 collectionId = keccak256(abi.encodePacked(msg.sender, salt));

Another possible solution is to use an incrementing collectionId that is stored in the contract and incremented each time a user calls _createCollection(). e.g. the first call to _createCollection() will have collectionId = 1 the second will have collectionId = 2 and so on.

Consider also removing the salt field from the creation of the coreCollection if the contract address is not required to be deterministic. This will use the CREATE opcode rather than CREATE2.

    address coreCollection = address(new CoreProxy{(collection));

[sofianeOuafir]

duplicate of #26