Badger-Vested-Aura contest - minhquanym's results

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/b6abb069431518962e1e0b3e516daa46ae3bdd9b/contracts/MyStrategy.sol#L219

Vulnerability details

Impact

Function _harvest() does multiple swaps from auraBAL -> BAL/ETH BPT -> WETH -> AURA using BalancerVault. But it doesn’t use minAmountsOut or have a check for mimimum return amount. It makes this function vulnerable to sandwich attack.

An attacker (which can be a malicious keeper) can front-run swap in BalancerVault before harvest() to make a profit.

Proof of Concept

Please refer to yDai Incident to check the severity of a harvest function without slippage control.

Please refer to Mushrooms-finance-theft to check how likely this kind of attack might happen.

Recommended Mitigation Steps

Should add a min return param or check the slippage.

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/b6abb069431518962e1e0b3e516daa46ae3bdd9b/contracts/MyStrategy.sol#L185

Vulnerability details

Impact

Function withdrawToVault() in BaseStrategy will withdraw all funds from strategy to vault, it uses an internal function _withdrawAll() in MyStrategy. In this function, there is a check that no locked balance is still in AuraLocker.

An attacker can keep deposit small amount (1 wei) for MyStrategy in AuraLocker by calling lock(address(MyStrategy), 1) in AuraLocker to keep locked balance of MyStrategy never be 0, making the function withdrawToVault() being in denial of service.

Proof of Concept

1. Attacker keep calling lock(address(MyStrategy), 1) in AuraLocker every lockDuration.
2. balanceOfPool() always bigger than zero making the check in line 185 revert.
3. Vault calls withdrawToVault() will always revert

Recommended Mitigation Steps

Add manualProcessExpiredLocks in function _withdrawAll() and remove the check if no locked balance.

1. Inconsistent usage of whenNotPaused modifier

In MyStrategy contract, there are 2 functions (manualProcessExpiredLocks and performUpkeep) basically do the same thing which is call processExpiredLocks() on LOCKER. Function manualProcessExpiredLocks() has whenNotPaused modifier but performUpkeep doesn’t.

Proof of concept

https://github.com/Badger-Finance/vested-aura/blob/b6abb069431518962e1e0b3e516daa46ae3bdd9b/contracts/MyStrategy.sol#L372

https://github.com/Badger-Finance/vested-aura/blob/b6abb069431518962e1e0b3e516daa46ae3bdd9b/contracts/MyStrategy.sol#L391

Recommended Migitation Steps

Add or remove whenNotPaused for both functions to keep consistency

2. Should use balanceOfWant() consistently

In MyStrategy contract, function balanceOfWant() simply return balance of want token in this strategy.

But instead of using it, this code do call balanceOf manually

uint256 toDeposit = IERC20Upgradeable(want).balanceOf(address(this));

Proof of concept

https://github.com/Badger-Finance/vested-aura/blob/b6abb069431518962e1e0b3e516daa46ae3bdd9b/contracts/MyStrategy.sol#L362

Recommended Mitigation Steps

Should change to use balanceOfWant() function.