TokenFactory.sol: DEFAULT_ADMIN_ROLE has wrong value

[code423n4]

Handle

hickuphh3

Vulnerability details

Impact

TokenFactory.sol defines DEFAULT_ADMIN_ROLE = keccak256("DEFAULT_ADMIN_ROLE");, but OpenZeppelin's AccessControl.sol defines DEFAULT_ADMIN_ROLE = 0x00, so that by default, all other roles defined will have their admin role to be DEFAULT_ADMIN_ROLE.

This makes the following lines erroneous:

// Give minter roles
SyntheticToken(syntheticToken).grantRole(DEFAULT_ADMIN_ROLE, longShort);
SyntheticToken(syntheticToken).grantRole(MINTER_ROLE, longShort);
SyntheticToken(syntheticToken).grantRole(PAUSER_ROLE, longShort);

// Revoke roles
SyntheticToken(syntheticToken).revokeRole(DEFAULT_ADMIN_ROLE, address(this));
SyntheticToken(syntheticToken).revokeRole(MINTER_ROLE, address(this));
SyntheticToken(syntheticToken).revokeRole(PAUSER_ROLE, address(this));

Due to how grantRole() and revokeRole() works, the lines above will not revert. However, note that TokenFactory will have DEFAULT_ADMIN_ROLE (0x00) instead of LongShort. This by itself doesn't seem to have any adverse effects, since TokenFactory doesn't do anything else apart from creating new synthetic tokens.

Nonetheless, I believe that DEFAULT_ADMIN_ROLE was unintentionally defined as keccak256("DEFAULT_ADMIN_ROLE"), and should be amended.

The revoking role order will also have to be swapped so that DEFAULT_ADMIN_ROLE is revoked last.

Recommended Mitigation Steps

bytes32 public constant DEFAULT_ADMIN_ROLE = 0x00;

function createSyntheticToken(
  string calldata syntheticName,
  string calldata syntheticSymbol,
  address staker,
  uint32 marketIndex,
  bool isLong
) external override onlyLongShort returns (ISyntheticToken syntheticToken) {
	...
  // Revoke roles
  _syntheticToken.revokeRole(MINTER_ROLE, address(this));
  _syntheticToken.revokeRole(PAUSER_ROLE, address(this));
	_syntheticToken.revokeRole(DEFAULT_ADMIN_ROLE, address(this));
}

[JasoonS]

Thanks they changed that more recently (or bad copy pasting...)