Strategist can make atomic changes that benefit him/her

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L611-L616 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L625-L636 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L645-L656 https://github.com/code-423n4/2022-04-badger-citadel/blob/main/src/StakedCitadel.sol#L664-L669

Vulnerability details

Impact

Strategist can make some changes to the protocol based on transactions seen in mempool or his own transactions. He can change governance fee to 0 when making a withdrawal. Or set his own fee to the max allowed if a large withdrawal is seen in the mempool.

Proof of Concept

When strategist sees a large reportHarvest(...) or reportAdditionalToken(...), he can sandwich this transaction with a front run that changes performanceFeeStrategist to the max allowable amount, and a backrun that changes it back. This will not be noticed by anyone unless they are tracking the events and can potentially allow the strategist to take a larger cut from big harvest report for a while.

Tools Used

Manual analysis

Recommended Mitigation Steps

I recommend using a short timelock, maybe even just for a few blocks, for any changes that can be made by a single person. This can be implemented by creating a strategist contract that is controlled by the strategist.

[GalloDaSballo]

I can't fully disagree because the strategist can indeed change these parameters, but I must emphasize how they have limits that are hard enforced by the contract (see maxPerforanceFee, etc..)

For that reason I believe the risk is minimized within well reasoned parameters

[GalloDaSballo]

As in, by default the max performance fee is 30%

What if governance sets that to 1%? Yes indeed the strategist can change from 0 to 1 (infinite values they can choose), but the limits are enforced by an actual timelocked multisig and allow the strategist to be a faster multisig

[jack-the-pug]

It's a QA