veToken Finance contest - cogitoergosumsw's results

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L117-L120

Vulnerability details

Impact

In VeAssetDepositor.sol#L117-L120, the condition to mint the additional rewards tokens to the user is if (incentiveVeAsset > 0). However, the incentiveVeAsset variable is only updated to zero after an external call to the ITokenMinter contract. This lacks the Checks Effects and Interactions safety pattern. In the event that the wrong minter contract has been initialised, an attacker could potentially drain all the additional reward tokens via a reentrancy attack.

Proof of Concept

• https://fravoll.github.io/solidity-patterns/checks_effects_interactions.html

Recommended Mitigation Steps

Be sure to follow the Checks Effects and Interactions safety pattern and update the incentiveVeAsset = 0 before minting the token for the user. Alternatively, the developers can also add the nonReentrant() modifier from OpenZeppelin to prevent any sort of potential reentrancy attacks.

Changing public constants to non-public

Reducing from public to private or internal can save gas when a constant isn’t used outside of its contract. I suggest changing the visibility from public to internal or private here:

uint256 public constant duration = 7 days; - BaseRewardPool.sol#L57

uint256 public constant newRewardRatio = 830; - BaseRewardPool.sol#L73

uint256 public constant MaxFees = 2000; - Booster.sol#L33

uint256 public constant FEE_DENOMINATOR = 10000; - Booster.sol#L34

> 0 is less gas efficient than != 0 for unsigned integers

!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas) with optimizer switched on, less gas is used.

Use != instead for unsigned integer comparisons

For example,

BaseRewardPool.sol:173: require(_amount > 0, "RewardPool : Cannot stake 0");

Target Codebase

BaseRewardPool.sol#L173

BaseRewardPool.sol#L196

BaseRewardPool.sol#L215

Booster.sol#L541

Booster.sol#L551

Booster.sol#L556

Booster.sol#L562

Booster.sol#L586

Booster.sol#L590

ExtraRewardStashV1.sol#L97

ExtraRewardStashV1.sol#L111

ExtraRewardStashV1.sol#L125

ExtraRewardStashV1.sol#L156

ExtraRewardStashV2.sol#L106

ExtraRewardStashV2.sol#L219

ExtraRewardStashV3.sol#L74

ExtraRewardStashV3.sol#L132

VE3DLocker.sol#L520

VE3DLocker.sol#L670

VE3DLocker.sol#L679

VE3DLocker.sol#L723

VE3DLocker.sol#L781

VE3DRewardPool.sol#L210

VE3DRewardPool.sol#L234

VE3DRewardPool.sol#L253

VE3DRewardPool.sol#L285

VeAssetDepositor.sol#L117

VeAssetDepositor.sol#L132

VeAssetDepositor.sol#L138

VoterProxy.sol#L100

Address can be set to immutable type

Some variables can be set to immutable type since they will only ever be set once. This can help to save gas.

Target Codebase

address public stakerLockRewards; - Booster.sol#L48

address public stakerLockRewards; - Booster.sol#L53

uint256 public pid; - ExtraRewardStashV1.sol#L23

address public operator - ExtraRewardStashV1.sol#L24

uint256 public pid - ExtraRewardStashV1.sol#L23

address public staker - ExtraRewardStashV1.sol#L25

address public gauge; - ExtraRewardStashV1.sol#L26

address public rewardFactory; - ExtraRewardStashV1.sol#L27

address public owner = msg.sender - Migrations.sol#L5

string private _name - VE3DLocker.sol#L109

string private _symbol - VE3DLocker.sol#L110

string public name; - VoterProxy.sol#L30

Redundant assignment of Variables

Some gas can be saved when less variables are assigned. In the examples below, the variables declared are not reused in other parts of the code.

Can be replaced with

    //withdraw from gauge
    try IStaker(staker).withdrawAll(pool.lptoken, pool.gauge) {
        pool.shutdown = true;
    } catch {}

Target Codebase

Booster.sol:53: address token = pool.lptoken; - Booster.sol#L337

Booster.sol:53: address gauge = pool.gauge; - Booster.sol#L337