hats-finance / Fenix--0x9d7765a7ebd5b6322a30797a44a5428531970d3d

0 stars 1 forks source link

Contracts which will use multiple times are not configured to claim gas yield #20

Open hats-bug-reporter[bot] opened 2 months ago

hats-bug-reporter[bot] commented 2 months ago

Github username: -- Twitter username: arniesec Submission hash (on-chain): 0x28b6eb5deeb568f70ac13b0438cc6f51ab431863c5f72242495e2577e035bf61 Severity: high

Description: Description\ on blast contracts acrrue yield when theybare used, multiple contracgs on fenix protocol which will be used more than once will accrue yield but this yield is not claimable because it is not configured correctly

Description\ the protocol will lose out on eth because it cannot claim gas yield

ArnieSec commented 2 months ago

Description

Smart contracts must interact with the Blast contract located at 0x4300000000000000000000000000000000000002 to change their Gas Mode.

By default the gas mode is set to void, in order to claim gas yields you must set it to claimable. This should be done in the constructor by calling configureClaimableGas

Recommendation

interface IBlast {
  // Note: the full interface for IBlast can be found below
  function configureClaimableGas() external;
  function configureGovernor(address governor) external;
}

contract MyContract {
  IBlast public constant BLAST = IBlast(0x4300000000000000000000000000000000000002);

  constructor(address governor) {
    BLAST.configureClaimableGas();
    // This sets the contract's governor. This call must come last because after
    // the governor is set, this contract will lose the ability to configure itself.
    BLAST.configureGovernor(governor);
  }
}

A set up like the one above will allow the claiming of gas yield, please ensure the governor is an EOA or a contract which has functions that can call function to claim gas yields.

ArnieSec commented 2 months ago

Currently there is a BlastGovernorClaimableSetup which does set the governor. There are 2 problems here

  1. Not all contracts set the governor

  2. The setup is missing a call to configureClaimableGas. Until the governor sets the gas mode manually, all yield will be lost

From blast docs

Contracts have two options for their Gas Mode:

Void (DEFAULT): base + priority fees go to the sequencer operator Claimable: base + priority fees spent on this contract can be claimed by the contract, net of L1 fees

As we can see since the contract is not configured first, the gas yield will end up going to the sequencer operator.

BohdanHrytsak commented 1 month ago

Hello. Thank you for your submission.

This behavior was intentional. Since a certain governor is configured in the contracts.

This address has the right to configure the modes in contracts, where it is specified as the governor.

In a way, it was left to the governor to determine whether they want to collect gas or not.

After deploying, the configuration of Claimable Gas for a particular contract was done by a separate call to the Blast contract from the specified governor

Blast.configureClaimableYieldOnBehalf(contractAddress, {from: governor})

The main point is that the said governor has the right to set regimes for contracts that have already been sealed. So we don't have to do anything in the constructor, although there are certain disadvantages

After that, it was decided that the approach with automatic setup was more preferable (in the first governor address, it could forget that it wanted to set Claimable Gas), so the implementation of BlastGovernorClaimableSetup was added for new contracts.

But all this, if necessary, to install Claimablae Gas can be done directly by calling Blast from this address, and does not require additional calls from the constructor

Thus, gas will not be lost, as the configuration of this contract provides for the possibility of setting Claimable Gas Mode at any time

ArnieSec commented 1 month ago

If a governor intends to set yield claimable from the beginning, he will be forced to lose out on gas collection of the tx that deployed the contracted.

To explain further, unless the contract is not set to claimable in the constructor, it is impossible to collect the gas from that tx. So if the governor did intend to have gas as claimable, he would be forced to lose out on the yield of the first tx because he would have to make another tx to activate the yield.

Therefore even in the scenario where the governor configures the claimable gas right away, he will still be unable to claim the gas from the first tx, therefore it is a loss of funds. My submission shows how it is impossible for the governor to collect the first tx of gas yield even if he wanted to.

This should classify it as a valid bug. Loss of funds, because the admin/governor cannot claim those funds if he wanted claimable gas from the start.

After deploying, the configuration of Claimable Gas for a particular contract was done by a separate call to the Blast contract from the specified governor

Even with the statements above, the loss is still seen, because of the use of a second call/tx. We were unable to harvest the yield from the first tx on the contract.

This means that even with that configuration we will not be able to claim the gas yield from the first tx, unless we configure claimable gas in the constructor.

@BohdanHrytsak

BohdanHrytsak commented 1 month ago

Having read the submission again, I have the following notes:

  1. All new contracts included in Scope use BlastGovernorClaimableSetup which solves the above problem
  2. For the existing contracts VotingEscrowV1_2, VoterV1_2, since they are already deployed, this change was not required, since Gas mode was set manually

Accordingly, taking into account the Scope, this issue is not valid

  1. Also, I wanted to make it clear that the decision to simply set governor was made to simplify things, and also to postpone the choice of gas intake, etc. Therefore, it is not very correct to count as a loss of assets before the decision to claim gas. Although, in the context of new contracts that already have BlastGovernorClaimableSetup, we took this into account, since the decision to always claim gas

For my part, I have no further comments on this submission

Thank you

0xmahdirostami commented 1 month ago

Lead: Thanks @ArnieGod , i agree with @BohdanHrytsak . It is designed to let the governor decide on claiming the gas yield in the Blast ecosystem, so it's invalid.