Badger-Vested-Aura contest - oyc_109's results

Lines of code

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

Vulnerability details

no slippage check

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

in the function _harvest() it makes a call ExitPoolRequest() with minAmountsOut for both assets set to 0

recommend specifying a proper minAmountsOut rather than 0

using old solidity version

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

recommend using a more up to date version of solidity

missing checks for zero address

description

Checking addresses against zero-address during initialization or during setting is a security best-practice. However, such checks are missing in address variable initializations/changes in many places.

Impact: Allowing zero-addresses will lead to contract reverts and force redeployments if there are no setters for such address variables.

findings

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

use of magic numbers

description

constants should be declared rather than use magic numbers

findings

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

open TODOs

description

Open TODOs can hint at programming or architectural errors that still need to be fixed.

Recommend resolving the TODO and bubble up the error.

findings

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L284 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L422

missing/incomplete NATSPEC

description

function parameters are not documented with proper natspec

anyone can send ETH

the receive() function has the following comment

/// @dev Can only receive ether from Hidden Hand

however anyone can send ETH to this contract as long as it is claiming

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L435-L438

for loop optimisations

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecessary gas.

Suggest not initializing the for loop counter to 0.

An array’s length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Suggest storing the array’s length in a variable before the for-loop, and use it instead:

++i costs less gas compared to i++

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

Suggest using ++i instead of i++ to increment the value of an uint variable.

Increments can be unchecked

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

taking all of the above, the recommended format for gas savings is

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

findings

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L118 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L153 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L300 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L317