Platform: Code4rena
Start Date: 15/06/2022
Pot Size: $30,000 USDC
Total HM: 5
Participants: 55
Period: 3 days
Judge: Jack the Pug
Id: 138
League: ETH
Rank: 25/55
Findings: 3
Award: $131.29
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: Picodes
Also found by: 0x1f8b, 0x52, Chom, GimelSec, IllIllI, berndartmueller, cccz, defsec, georgypetrov, hyh, kenzo, minhquanym, oyc_109, scaraven, unforgiven
50.7077 USDC - $50.71
https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L249 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L275
Single swaps of _harvest contains no slippage or deadline, which makes it vulnerable to sandwich attacks, MEV exploits and may lead to significant loss of yield.
When using BALANCER_VAULT.swap here and here, there is no slippage protection. Therefore a call to _harvest generating swaps could be exploited for sandwich attacks or other MEV exploits such as JIT.
The scenario would be:
A authorized actor calls harvest, leading to a swap of say x auraBAL to BAL/ETH BPT and then y WETH to BAL.
Then while the transaction is in the mempool, it is exploited for example like in https://medium.com/coinmonks/defi-sandwich-attack-explain-776f6f43b2fd
The easiest mitigation would be to pass a minimum amount of AURA that the swap is supposed to get in harvest. It should not add security issues as callers of harvest are trusted.
An other solution would be to do like here to use Cowswap for example, or any other aggregator.
#0 - GalloDaSballo
2022-06-22T00:40:15Z
Dup of #155
#1 - GalloDaSballo
2022-06-22T00:40:41Z
I love how the warden linked my code to integrate cowswap XD
#2 - GalloDaSballo
2022-07-13T22:30:47Z
Confirmed and mitigated in 2 ways:
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xFar5eer, 0xNazgul, 0xNineDec, 242, Chom, Czar102, Funen, GimelSec, Meera, Picodes, Sm4rty, Tadashi, TerrierLover, Waze, _Adam, a12jmx, asutorufos, codexploder, cryptphi, defsec, gzeon, hyh, joestakey, minhquanym, oyc_109, reassor, robee, saian, sorrynotsorry, unforgiven, zzzitron
51.2645 USDC - $51.26
https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L372 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L391
Pausing the contract should stop all permissionless functionalities. But the function performUpkeep lacks whenNotPaused, so keepers will still be able to call it and process expired locks even if the contract is paused, leading to a loss of fund as keeper would still run, and preventing the contract from being fully paused as intended.
From the function manualProcessExpiredLocks, we see that calling processExpiredLocks should be forbidden when the contract is paused. But the exact same function does not contains the whenNotPaused modifier: https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L391
Add the whenNotPaused modifier or reuse manualProcessExpiredLocks in performUpkeep
#0 - GalloDaSballo
2022-06-19T01:01:28Z
Disagree with severity in lack of a POC, allowing to unlock is a good thing, if anything we should remove the extra checks
29.3239 USDC - $29.32
[GAS - 01] In claimBribesFromHiddenHand, token could be reused
The same external call is made twice:
https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L318 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L301
Therefore gas could saved using a memory array to avoid doing the same external call twice.
#0 - GalloDaSballo
2022-06-19T01:02:14Z
token is the address of the token we got from the claim identifier, disagree that it can be reused
#1 - Picodes
2022-06-24T20:18:02Z
@GalloDaSballo to clarify, my suggestion would be to store all the token results of line 301 in an array to reuse this array on line 318
#2 - GalloDaSballo
2022-06-24T21:46:36Z
That is correct and would save 100 + 100 gas (give or take) per token as each time we are STATIC_CALLing (100 gas) to get the token back and the SLOAD would be from hot slot (100 gas)