Race between governance and strategist on other token earned

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L439 https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L698

Vulnerability details

Impact

There is a race between the strategist and the governance to report other tokens earned by the strategy. Indeed the strategist can trigger the function (1) by calling the strategy while the governance can call (2). Both these functions can report earn tokens. But (1) will distribute revenue to the strategist, while the (2) will distribute revenue only to the governance. They both have interest to record new revenue first.

Proof of Concept

hardhat test

Tools Used

(1) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L439 (2) https://github.com/code-423n4/2022-04-badger-citadel/blob/18f8c392b6fc303fe95602eba6303725023e53da/src/StakedCitadel.sol#L698

Recommended Mitigation Steps

In the function (2) distribute part of the revenues to the strategist (in the same proportion as function (1))

[GalloDaSballo]

The ordinary strategy in the vault would be using reportAdditionalToken so the strategy set by governance would have a clear path that also includes taking the fees.

In the case that an exceptional token (nonProtected) is received, then it can be sweeped at discretion of the strategist to the governance address for any use they deem fit.

I don't believe there's a race condition but rather two possibilities.

Personally would like to contest in lack of the nuance that protectedTokens from the strategy would protect from sweeping a random token.