PrePOMarketFactory.sol : createMarket is allowing to create market that is already created

[code423n4]

Lines of code

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarketFactory.sol#L22-L34

Vulnerability details

Impact

This will overwrite the existing market and erase the existing one.

Proof of Concept

https://github.com/prepo-io/prepo-monorepo/blob/3541bc704ab185a969f300e96e2f744a572a3640/apps/smart-contracts/core/contracts/PrePOMarketFactory.sol#L22-L34

  function createMarket(string memory _tokenNameSuffix, string memory _tokenSymbolSuffix, bytes32 longTokenSalt, bytes32 shortTokenSalt, address _governance, address _collateral, uint256 _floorLongPrice, uint256 _ceilingLongPrice, uint256 _floorValuation, uint256 _ceilingValuation, uint256 _expiryTime) external override onlyOwner nonReentrant {
require(validCollateral[_collateral], "Invalid collateral");

(LongShortToken _longToken, LongShortToken _shortToken) = _createPairTokens(_tokenNameSuffix, _tokenSymbolSuffix, longTokenSalt, shortTokenSalt);
bytes32 _salt = keccak256(abi.encodePacked(_longToken, _shortToken));

PrePOMarket _newMarket = new PrePOMarket{salt: _salt}(_governance, _collateral, ILongShortToken(address(_longToken)), ILongShortToken(address(_shortToken)), _floorLongPrice, _ceilingLongPrice, _floorValuation, _ceilingValuation, _expiryTime);
deployedMarkets[_salt] = address(_newMarket);

_longToken.transferOwnership(address(_newMarket));
_shortToken.transferOwnership(address(_newMarket));
emit MarketAdded(address(_newMarket), _salt);

}

From above function, salt is computed using the short and long token as shown below,

bytes32 _salt = keccak256(abi.encodePacked(_longToken, _shortToken));

The _longToken and _shortToken are obtained is using the name data using the function _createPairTokens

        function _createPairTokens(string memory _tokenNameSuffix, string memory _tokenSymbolSuffix, bytes32 _longTokenSalt, bytes32 _shortTokenSalt) internal returns (LongShortToken _newLongToken, LongShortToken _newShortToken) {
string memory _longTokenName = string(abi.encodePacked("LONG", " ", _tokenNameSuffix));
string memory _shortTokenName = string(abi.encodePacked("SHORT", " ", _tokenNameSuffix));
string memory _longTokenSymbol = string(abi.encodePacked("L", "_", _tokenSymbolSuffix));
string memory _shortTokenSymbol = string(abi.encodePacked("S", "_", _tokenSymbolSuffix));
_newLongToken = new LongShortToken{salt: _longTokenSalt}(_longTokenName, _longTokenSymbol);
_newShortToken = new LongShortToken{salt: _shortTokenSalt}(_shortTokenName, _shortTokenSymbol);
return (_newLongToken, _newShortToken);

}

It is possible to craft the long and short token that can procude the same salt value.

Refer the following link for an example on how same hash can be created using two different data https://forum.openzeppelin.com/t/the-trap-of-using-encodepacked-in-solidity/1052

Though the function is under owner control, still I want to raise this issue by considering the impact that would create on end user or even if by mistake user end with such hash collision.

Tools Used

Manual review

Recommended Mitigation Steps

Though the function is under

uint nonce;
function createMarket(string memory _tokenNameSuffix, string memory _tokenSymbolSuffix, bytes32 longTokenSalt, bytes32 shortTokenSalt, address _governance, address _collateral, uint256 _floorLongPrice, uint256 _ceilingLongPrice, uint256 _floorValuation, uint256 _ceilingValuation, uint256 _expiryTime) external override onlyOwner nonReentrant {
require(validCollateral[_collateral], "Invalid collateral");

   +++require(deployedMarkets[_salt] == address(0)); ------------------------------------> add check

(LongShortToken _longToken, LongShortToken _shortToken) = _createPairTokens(_tokenNameSuffix, _tokenSymbolSuffix, longTokenSalt, shortTokenSalt);

 +++bytes32 _salt = keccak256(abi.encodePacked(_longToken, _shortToken, nonce++)); -----------------------> add nonce

PrePOMarket _newMarket = new PrePOMarket{salt: _salt}(_governance, _collateral, ILongShortToken(address(_longToken)), ILongShortToken(address(_shortToken)), _floorLongPrice, _ceilingLongPrice, _floorValuation, _ceilingValuation, _expiryTime);
deployedMarkets[_salt] = address(_newMarket);

_longToken.transferOwnership(address(_newMarket));
_shortToken.transferOwnership(address(_newMarket));
emit MarketAdded(address(_newMarket), _salt);

}

[Picodes]

https://ethereum.stackexchange.com/questions/97340/can-create2-with-the-same-salt-override-existing-contract

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[aktech297]

@Picodes

Through this issue I am trying to point out the possible hash collision while using two variable. I am not referring about the CREATE2. When we look at the code,

(LongShortToken _longToken, LongShortToken _shortToken) = _createPairTokens(_tokenNameSuffix, _tokenSymbolSuffix, longTokenSalt, shortTokenSalt); bytes32 _salt = keccak256(abi.encodePacked(_longToken, _shortToken));

_salt is the result of keccak256 of long and short token.

Every new market is stored in a map using the _salt as key.

deployedMarkets[_salt] = address(_newMarket);

It is possible to get same _salt for different combination, refer the link https://forum.openzeppelin.com/t/the-trap-of-using-encodepacked-in-solidity/1052

So, existing market in the map deployedMarkets[_salt] can be overwritten.

[Picodes]

• there is no dynamic input in the call to encodePacked so hash collision does not work in this case
• even if you manage to produce the same salt the next like will revert due to the create2 opcode

Please do a coded PoC if you want to argue

[aktech297]

I am not sure my explanation is well received. Again I wanted to mention that the issue is not related to create2 opcode.

The issue is related to hashing using keccak256(abi.encodePacked(_longToken, _shortToken));

I have already referred a POC, https://forum.openzeppelin.com/t/the-trap-of-using-encodepacked-in-solidity/1052

Can't we get same hash for two different pair of inputs ?

You already pointed that same salt can be used to create two different contracts with different args. https://ethereum.stackexchange.com/questions/97340/can-create2-with-the-same-salt-override-existing-contract

So, your statement even if you manage to produce the same salt the next like will revert due to the create2 opcode is not correct.

[Picodes]

1 - Hash collision when using abi.encodePacked is a well-known issue, but you need dynamic types so it does not apply here: neither _longToken nor _shortToken are dynamic types.

2 - Even if 1 wasn't there, please show me how _newLongToken = new LongShortToken{salt: _longTokenSalt}(_longTokenName, _longTokenSymbol); _newShortToken = new LongShortToken{salt: _shortTokenSalt}(_shortTokenName, _shortTokenSymbol); and PrePOMarket _newMarket = new PrePOMarket{salt: _salt}(_governance, _collateral, ILongShortToken(address(_longToken)), ILongShortToken(address(_shortToken)), _floorLongPrice, _ceilingLongPrice, _floorValuation, _ceilingValuation, _expiryTime); doesn't revert if the _salt has already been used

Please do a coded PoC so you can see for yourself why it doesn't work or if you are super confident and want to argue more.

[aktech297]

@Picodes Sorry for delay. As I was working on another contest, could not find time to spend on this.

I have tried simple POC and the codes are, I have used minimal amount of inputs for better understanding.

// SPDX-License-Identifier: MIT pragma solidity ^0.8.17;

import "./PrePoMarket.sol";

contract PrePoMarketFactory { mapping(bytes32 => address) public deployedMarkets;

string public longTok1 = "aaa";
string public longTok2 = "aaab";

string public shortTok1 = "bcc";
string public shortTok2 = "cc";

bytes32 public _salt1 = keccak256(abi.encodePacked(longTok1, shortTok1));
bytes32 public _salt2 = keccak256(abi.encodePacked(longTok2, shortTok2));

PrePoMarket public _newMarket1 = new PrePoMarket{salt: _salt1}(longTok1, shortTok1);

PrePoMarket public _newMarket2 = new PrePoMarket{salt: _salt2}(longTok2, shortTok2);

}

pragma solidity ^0.8.17;

contract PrePoMarket {

constructor(string memory longToken, string memory ShortToken){

}

}

The outputs are

salts both are same

salt1 = 0x4f21a3cc924b7692f9c0942ef02043d4d8d7bc114f74e95a3e9d25cceb8e8e18 salt2 = 0x4f21a3cc924b7692f9c0942ef02043d4d8d7bc114f74e95a3e9d25cceb8e8e18

Markets are both are different

newMarket1 = 0xd97bdd99fDEdce415BC966F952292151DD31dDaF newMarket2 = 0x5f9C669231266c6804941Eb9A783E035481147dA

From above POC, it is clear usage of same salt will not revert if the arguments are different.

let me know if you still not clear on this. Please DM.

Remix is used for testing.

[Picodes]

Hi @aktech297, I still think you are wrong because _longToken and shortToken are adresses and not strings as I explained earlier.

Can you do the same with longToken and shortToken being adresses and not strings? You can ping me on discord if needed, haven't found you as I think your discord pseudo and your github one are differents

[aktech297]

@Picodes

Are you sure about it??..

I don't see any difference between whats used as arguments. i believe it would not revert

ak1 is my discord handle.

[bin2chen66]

@aktech297

My personal opinion.

Are you thinking that this slat has the same possibility?

bytes32 _salt = keccak256(abi.encodePacked(_longToken, _shortToken));

If _salt is to be the same, then _longToken and _shortToken must be the same as sometime before

The _longToken is an impossible duplicate because it is a contract address, and it is a newly generated contract each time, so if it is the same address as the previous one, it will not execute successfully. This place will fail

  function _createPairTokens(string memory _tokenNameSuffix, string memory _tokenSymbolSuffix, bytes32 _longTokenSalt, bytes32 _shortTokenSalt) internal returns (LongShortToken _newLongToken, LongShortToken _newShortToken) {
.....
    _newLongToken = new LongShortToken{salt: _longTokenSalt}(_longTokenName, _longTokenSymbol);
....
  }

[aktech297]

Hi @bin2chen66

Thanks for taking a look and sharing your points

we don't need same variable always to produce same salt. Even with different combination of inputs we can produce the same salt. I have shared some secnario where it could happen .

If that happens, when we already have a market that's is stored in mapping using the salt, anyone can do some research and come up with contracts which can produce same salt as before..

The current implementation is updating the map without checking whether any market is already available .

[bin2chen66]

@aktech297

The biggest difference in your example is that “_longToken“ and "_shortToken" is not a string,if it is a string you are right. But it is contract address , then the problem will not occur, because when it occurs, the whole createMarket() will revert, as I explained in the previous one

[aktech297]

Well the way i look at them as letter whether string or contract . Anything that we give is gonna return as bytes from keccak256. I don't see any reason if it is contract to produce the same hash using keccak256.

[aktech297]

I had reported my findings it's upto the judge and protocol to consider. I am stopping my comments here.

[Picodes]

Thank you @bin2chen66 for commenting as well. As I said in this comment, the variables are addresses and not string so there is no hash collision possibility. Therefore I am sure that this finding is not valid, and in the absence of a PoC in the context of the contest I won't reconsider my decision.

[Picodes]

This being said, @aktech297 please recall that the rule is the following: https://discord.com/channels/810916927919620096/976603323450941440/1041784972807241818, and that from scratch I asked about a coded PoC (in the context of the contest, not a generic PoC about abi.encodePacked).

[aktech297]

@Picodes Thanks for letting me know the rule.. Rule is rule.. i will follow.