Badger-Vested-Aura contest - 0x1f8b's results

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L249 https://dev.balancer.fi/resources/swaps/single-swap#swap-function

Vulnerability details

Impact

Due to a bug with the slippage configuration it is possible to make a call to the harvest method do a swap with 0 BALETH_BPT tokens.

Proof of Concept

According to the balancer documentation the limit argument represents:

limit: The meaning of limit depends on the value of singleSwap.kind GIVEN_IN: The minimum amount of tokens we would accept to receive from the swap.

So if an attacker detects the harvest transaction in the memory pool, and configures two transactions to perform a sadwitch attack, the contract will perform a swap with an expected minimum of 0 tokens.

Reference:

• https://dev.balancer.fi/resources/swaps/single-swap#swap-function

Affected source code:

• MyStrategy.sol#L249

Recommended Mitigation Steps

• Use a percentage for the slippage

Low

Obsolete pragma

The pragma version used is:

pragma solidity 0.6.12;

But recently solidity released a new version with important Bugfixes:

• The first one is related to ABI-encoding nested arrays directly from calldata. You can find more information here.

• The second bug is triggered in certain inheritance structures and can cause a memory pointer to be interpreted as a calldata pointer or vice-versa. We also have a dedicated blog post about this bug.

Apart from these, there are several minor bug fixes and improvements.

The minimum required version should be 0.8.14

Emit event for important state changes

It's important to emit events when the governance change important values in the contract, in order to be more open and transparent.

Affected source code:

• MyStrategy.sol#L86-L101

Lack of visibility

The method claimBribesFromHiddenHand does not define the public visibility, take in care that when is not defined, the visibility is public.

Affected source code:

• MyStrategy.sol#L288

Lack of whenNotPaused

The method claimBribesFromHiddenHand allows to do do changes when the contract is paused. It's better to limit this.

Affected source code:

• MyStrategy.sol#L288

Open ToDo

TODO usually mean something still have to be checked of done. This could lead to vulnerabilities if not verified.

Affected source code:

• MyStrategy.sol#L284
• MyStrategy.sol#L422

Non-critical

No gap used

Although it is a contract that is not inherited, it is possible that it will be extended in the future, it is worth adding _gap variables at the end of the contract to avoid update problems.

Affected source code:

• MyStrategy.sol

Use external visibility

The following methods could be improved if external visibility is used:

• claimBribesFromHiddenHand

Logic optimization

In MyStrategy.sol#L56-L59 is not needed the argument, it's possible to change the logic from:

function initialize(address _vault) public initializer {
        assert(IVault(_vault).token() == address(AURA));
        __BaseStrategy_init(_vault);

to:

function initialize() public initializer {
        __BaseStrategy_init(AURA);

Obsolete pragma

Gas optimizations and additional safety checks are provided for free when using newer compiler versions and the optimizer.

pragma solidity 0.6.12;

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

If it's not possible to use error codes due to the pragma used, it is recommended to reduce the strings to less than 32 bytes.

Affected contracts:

• MyStrategy.sol#L186

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

It is also recommended to not initialize the counter variable and surround the increment with an unchecked region.

Affected source code:

• MyStrategy.sol#L118-L120
• MyStrategy.sol#L153-L155
• MyStrategy.sol#L300
• MyStrategy.sol#L317