Missing checks for same tokens in function createBribes will lead to the creation of incorrect bribes.

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x32b1231ae1dbe23129b3ae4263ac5525180437962b18f40a5feea37f7a5e5f26 Severity: medium

Description: Description\ The createBribes Function is missing the crucial check to ensure than token0 != token1 when creating bribes. This allows the creation of an incorrect bribe by voters. A situation where token0 == token1 will lead to the creation of an incorrect bribe.

    function createBribe(address _token0, address _token1, string memory _type) external returns (address) {
        require(msg.sender == voter || msg.sender == owner(), "only voter or voter");

        address newLastBribe = address(new BribeProxy());

        IBribe(newLastBribe).initialize(defaultBlastGovernor, voter, address(this), _type);

        if (_token0 != address(0)) IBribe(newLastBribe).addRewardToken(_token0);
        if (_token1 != address(0)) IBribe(newLastBribe).addRewardToken(_token1);

        IBribe(newLastBribe).addRewardTokens(defaultRewardToken);

        last_bribe = newLastBribe;

        return newLastBribe;
    }

Recommendation

Require that token0 is not the same as token1 before creating a bribe.

    function createBribe(address _token0, address _token1, string memory _type) external returns (address) {
        require(msg.sender == voter || msg.sender == owner(), "only voter or voter");
@>   require(_token0 != _token1);
        address newLastBribe = address(new BribeProxy());

        IBribe(newLastBribe).initialize(defaultBlastGovernor, voter, address(this), _type);

        if (_token0 != address(0)) IBribe(newLastBribe).addRewardToken(_token0);
        if (_token1 != address(0)) IBribe(newLastBribe).addRewardToken(_token1);

        IBribe(newLastBribe).addRewardTokens(defaultRewardToken);

        last_bribe = newLastBribe;

        return newLastBribe;
    }

[BohdanHrytsak]

Thank you for the submission.

An additional check at the stage of creating a bribe can only act as an additional safeguard, but in fact, it has no value

Let's look at what will happen and in what situations tokens can be the same:

• On the Bribe implementation will call: addRewardToken, which will not allow adding the same token: https://github.com/Satsyxbt/Fenix/blob/7b81d318fd9ef6107a528b6bd49bb9383e1b52ab/contracts/bribes/BribeUpgradeable.sol#L365C1-L366C1
• Setted through Bribe creation by the owner - misconfiguration, oos

[0xisaacc]

Hi @BohdanHrytsak,

Here are the answers to the points.

On the Bribe implementation will call: addRewardToken, which will not allow adding the same token: https://github.com/Satsyxbt/Fenix/blob/7b81d318fd9ef6107a528b6bd49bb9383e1b52ab/contracts/bribes/BribeUpgradeable.sol#L365C1-L366C1

The check there is to ensure that the reward token hasn't been added before. It's not checking to ensure that the tokens that the reward token consists of are not the same token. So, as long as the bribe reward token that consists of the same tokens hasn't been added before by the voter or the owner, the check there will pass and the incorrect bribe token will be created.

Setted through Bribe creation by the owner - misconfiguration, oos.

A voter who is not the owner can also create bribes.

require(msg.sender == voter || msg.sender == owner(), "only voter or voter");

https://github.com/Satsyxbt/Fenix/blob/7b81d318fd9ef6107a528b6bd49bb9383e1b52ab/contracts/bribes/BribeFactoryUpgradeable.sol#L33

Thanks.

[BohdanHrytsak]

@0xisaacc I don't have enough information to understand what you consider to be an incorrect bribe, could you please define what an incorrect bribe is?

The main task of the factory is to create a bribe, its main caller will be Voter

Since token0, token1 come from Voter, and Voter takes them from Vaults/Factories, etc., and is not sure that these tokens will be different from certain specific Vaults. Adding restrictions may make the situation worse

On the other hand, creating a Bribe when transferring the same tokens does not carry any risks as such, since the token can still be added as a bribe

[0xisaacc]

what you consider to be an incorrect bribe, could you please define what an incorrect bribe is?

@BohdanHrytsak The names tokens 0 and token 1 indicate that there should be two different tokens. An incorrect bribe would be any bribe token that doesn't adhere to this standard. E g creating same tokens bribes (token 0, token 0).

These are the irregularities that I'm trying to point out and I hope you see the point.

Here's a similar check in function create pair when creating a token pair.

if (tokenA == tokenB) {
            revert IdenticalAddress();
        }

https://github.com/Satsyxbt/Fenix/blob/7b81d318fd9ef6107a528b6bd49bb9383e1b52ab/contracts/dexV2/PairFactoryUpgradeable.sol#L131

Let there be a check when creating bribe tokens too.

[BohdanHrytsak]

Token names alone do not mean that they should be different

Why do you think that the wrong bribe will be one where these tokens are the same? Nothing will break, everything will work as expected, a support case is more desirable for us

As you mentioned, some of the factories that we will use have these require. But we will also be using specific contracts like IchiVault, etc., so there is no guarantee that we only need a case where these tokens are different and that this single use case is valid.

Therefore, still invalid