search

code-423n4 / 2022-06-badger-findings

0 stars 0 forks source link

Users can bypass processLocksOnReinvest #48

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L356

Vulnerability details

Impact

A malicious user can simply bypass the governance decision of processLocksOnReinvest by using prepareWithdrawAll function

Proof of Concept

  1. Governance has set processLocksOnReinvest as false

  2. Governance call reinvest function and assume that expired locks wont be reinvested

  3. Attacker frontrun the Governance transaction and simply calls prepareWithdrawAll function which process all expired locks

  4. Now after attacker transaction, governance transaction runs. Since expired lock balance is already present in contract so it gets reinvested even though governance never wished for same

Recommended Mitigation Steps

Do not allow users to process expired locks

GalloDaSballo commented 2 years ago

I fail to see this as a vulnerability, governance would process expired locks either to relock or to leave them unlocked in the strategy.

The fact that anyone can unlock (when the lock has expired mind you), is so that people have the freedom to withdraw from the vault when lock expire

Consider that we have a open function performUpkeep that is meant to unlock locks any time they expire

For those reasons, I have to disagree with any type of vulnerability here, governance can set that variable for it's convenience (always unlock and then re-lock, vs just lock whatever is liquid)