mstpr-brainbot - RubiconFeeController contract initializes the "baseFee" in constructor but the contract is upgradable

[sherlock-admin]

mstpr-brainbot

medium

RubiconFeeController contract initializes the "baseFee" in constructor but the contract is upgradable

Summary

RubiconFeeController contract is an upgradable contract but it tries to set the "baseFee" in the constructor. However, due to upgradable contracts not aware of the constructor state, the "baseFee" will not be updated.

Vulnerability Detail

In the code, "baseFee" is initially set to "10", but it is not defined as a constant or immutable value. Therefore, any updates made to it will not be reflected in the storage of the upgradable contracts. Consequently, the value of "baseFee" will remain as "0". Fillers who are fulfilling the orders can exploit this situation by executing the orders without incurring any fees.

Impact

When protocol notices that they set the baseFee as "0" they can use this function to set it to a desired value: https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/11cac67919e8a1303b3a3177291b88c0c70bf03b/gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol#L132-L136

However, until protocol team notices this all the orders filled will be used "0" fee which is not desired as we can see the set function can't set the fee to "0".

Considering protocol team didn't notice this, I'll label this as medium.

Code Snippet

https://github.com/sherlock-audit/2024-02-rubicon-finance/blob/11cac67919e8a1303b3a3177291b88c0c70bf03b/gladius-contracts-internal/src/fee-controllers/RubiconFeeController.sol#L16-L26

Tool used

Manual Review

Recommendation

Set the baseFee inside the initialize function

[sherlock-admin]

2 comment(s) were left on this issue during the judging contest.

tsvetanovv commented:

Low. I think this will be an admin mistake if he doesn't set the fee early

0xAadi commented:

Invalid: Admin can reset the baseFee later using setBaseFee

[daoio]

true, but disagree with severity, it should be low (because we have setBaseFee there)

[soliditywala]

I guess it should be medium, because protocol will be missing out on the fees until the admin manually reset the baseFees. This is a design issue by not initializing the baseFees in the initialize(). Because there also exist setFeeRecipient() to set the feeRecipient, then what is the point of setting this in the initialize() if it can also be set separately?

[ArnieSec]

should stay invalid, contracts can be deployed first and fees set before setting up the api that feeds order to fillers. No impact

[sherlock-admin]

The protocol team fixed this issue in PR/commit https://github.com/RubiconDeFi/gladius-contracts-internal/pull/15.

[sherlock-admin]

The Lead Senior Watson signed-off on the fix.