Protocol variables `incentiveOwnerB` and `incentiveOwnerA` can be locked permanently and protocol may lose funds.

[hats-bug-reporter[bot]]

Github username: @https://github.com/kodakr Twitter username: @Kodak_Rome Submission hash (on-chain): 0x4a3fd05a01d478c0d3130c6b1c03d8add726e94320df7bef8130b9e774a3fd3b Severity: high

Description: Description\ In FeeManager.sol, the functions changeIncentiveUSDA() and changeIncentiveUSDB() are external functions intended for settting the aforementioned state variables. Unfortunately, its implementations are too optimistic for a different-users-function (with time) as it allows a user to advertently or inadvertently grief protocol by setting either variable to address(0). This state will be highly undesirous to protocol.

Note that:

1. Function with time will be called by different users. (High chance of occurence at point T).
2. Both functions are vulnerable.
3. Only one function is needed to reach this undesired state (X2 chance of occurence).
4. Variables are utilised accross contract for fund accounting (Possible loss of fund).
5. These variable are significant to protocol

The full impact of this is yet to be uncovered. Be right bk ...

[vm06007]

Hello @kodakr

I think you've made few false assumption that does not take in account: 1) changeIncentiveUSDA and changeIncentiveUSDB can ONLY be called by incentiveOwnerA address and incentiveOwnerB address correspondingly, meaning it is only for internal team use to assign incentives to initial developers.

2) These function would change ownership ONLY if incentiveOwnerA and incentiveOwnerB choose to change address where assigned incentive would go.

3) Keep in mind that incentiveOwnerA and incentiveOwnerB are smart-contracts that has a wrapper on the call to the main contract preventing this to happen by checking passed value.

Furthermore, it is very low chance and unlikely to happen that incentiveOwnerA or incentiveOwnerB would overwrite wallets or targets where the incentives would be redirected. It is also not in their interest to forfeit any interest and just set those to 0x0 address for no good reason rather than abandoning protocol (self-destruct)

Marking as invalid. (this is definitely not a high to suggest adding 0x0 address check)

[kodakr]

Thanks for ur reply, Are u explicitly saying that:

1. Its absolutely technically impossible for our smart contract to reach that state (incentiveOwnerA == address(0) || incentiveOwnerB == address(0))????? If yes, then im glad to learn this and rest my case. Otherwise; Question 2

2. Are u saying that this state (irreversible) Is of no -ve significance to our protocol???

Note that there's no function to reverse this.

bool declaration = (Question1 == true || Question2 == true);

Dear @vm06007 is declaration true????

[vm06007]

1. If yes, then im glad to learn this and rest my case

Hey Henry, I'm saying that this is out of scope, since it depends which smart-contract address team will decide to put in control of FeeManager and incentiveOwnerA / incentiveOwnerB representation, thus this is centralization topic which you will find is mentioned several times in OOS definition. This is very similar case to arguing that master could call renounceOwnership() or could call createPool using malicious token.

declaration = false.

[kodakr]

Ok then, thanks for ur reply.

since it depends which smart-contract address team will decide to put in control of FeeManager and incentiveOwnerA / incentiveOwnerB representation.

Glad to learn this info will guide ur decision. Hence not an invalid report.

[kodakr]

Escalation: submission shouldn't retain the invalid tag. ( Perhaps an omission ). @vm06007 , @vonMangoldt , @Foon256

Severity = (Impact * Probability)

Hey Henry, I'm saying that this is out of scope, since it depends which smart-contract address team will decide to put in control .....

I understand u are yet to make a decision on wallet (msig/EOA etc) which i admit surely affect the severity of (this). However, my counter-argument is:

1. Since its a decision to be made, there's still Probability for each outcome (as at contest time). This finding brings u to awareness of this undesirous state. Hence will definitely impact that decision. Hence impacting the protocol positively.

2. Finally, do not forget that the said wallet-type-decision (even at its best outcome) only reduces the chance of occurrence and doesn't entirely make reaching that state a probability of 0. While its potential impact is still perfectly intact.

This is very similar case to arguing that master could call renounceOwnership() or ...

This is a very different scenario as:

1. Its wallet-type-decision is already made, in use and proof seen as implemented in test file const multisig.

That said, this finding has 0 possibility of "invalid"

Pls consider retagging

[vm06007]

Escalation: submission shouldn't retain the invalid tag. ( Perhaps an omission ). @vm06007 , @vonMangoldt , @Foon256

Severity = (Impact * Probability)

Hey Henry, I'm saying that this is out of scope, since it depends which smart-contract address team will decide to put in control .....

I understand u are yet to make a decision on wallet (msig/EOA etc) which i admit surely affect the severity of (this). However, my counter-argument is:

1. Since its a decision to be made, there's still Probability for each outcome (as at contest time). This finding brings u to awareness of this undesirous state. Hence will definitely impact that decision. Hence impacting the protocol positively.
2. Finally, do not forget that the said wallet-type-decision (even at its best outcome) only reduces the chance of occurrence and doesn't entirely make reaching that state a probability of 0. While its potential impact is still perfectly intact.

This is very similar case to arguing that master could call renounceOwnership() or ...

This is a very different scenario as:

1. Its wallet-type-decision is already made, in use and proof seen as implemented in test file const multisig.

That said, this finding has 0 possibility of "invalid"

Pls consider retagging

Henry, this is purely out of scope and clearly trying to manipulate a simple fact -> this is not a bug, incentiveOwner should be able to set it to arbitrary address, including 0x0 if incentive owner decides to abandon protocol and not to collect any more fees in future. By all means this will make FeeManager not to set any extra incentive for the owner if address been overwritten. Since now we are talking about centralization topic this entire submission falls out of place due to being already mentioned not to be in focus if you read carefully OOS section, we want submissions focusing on the bugs in code, not on theoretical scenarios which are intended to be carried out should there be such a need or not. This report is invalid.

Same as #20 and #21 etc - same topic oh what if master renounced ownership etc. Clearly this was already taken in account so this entire topic was mentioned in OOS section to be excluded.