veToken Finance contest - TerrierLover's results

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L315 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L360 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L387 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L406

Vulnerability details

Impact

underflow can happen in for loop at the following functions at VE3DLocker.sol in the certain conditions.

• balanceAtEpochOf
• pendingLockAtEpochOf
• totalSupply
• totalSupplyAtEpoch

Proof of Concept

The end condition of the problematic for loop is i + 1 != 0.

for (uint256 i = locks.length - 1; i + 1 != 0; i--)

To fulfill the end condition, i should be -1. However, i is uint, so i can't be minus. In the 0.8.7 version, when underflow happens, it issues an error. Therefore, when i is 0 in the for loop, it does not fulfill the end condition i + 1 != 0 so i-- is executed. Then the underflow happens which results in issuing an error.

Tools Used

Static code analysis

Recommended Mitigation Steps

Use int256 instead of uint256 for i so that i can be a minus value.

for (int256 i = locks.length - 1; i + 1 != 0; i--)

If i will not be a minus value in the affected codebase, it is still worth considering this corner case to prevent the unexpected error.

[QA-1] Use capital letters with underscores for constant

As solidity document mentions here, it should be named with all capital letters with underscores separating words. However, following state variables do not follow this pattern.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L57 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L73 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L33 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV1.sol#L21 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L20 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV3.sol#L20 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/StashFactory.sol#L19-L21 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L79 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L82 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L99 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L59 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L64 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeTokenMinter.sol#L15 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VirtualBalanceRewardPool.sol#L70

---

[QA-2] Naming inconsistency at function arguments

BaseRewardPool.sol and VE3DRewardPool.sol have naming inconsistency at function arguments.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L113

function balanceOf(address account)

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L121

function addExtraReward(address _reward)

balanceOf function does not have _ in its argument while addExtraReward function has _ in its argument. Function arguments at other files have _ at their prefixes. So BaseRewardPool.sol should also follow this pattern.

Except for BaseRewardPool.sol and VE3DRewardPool.sol, following functions also have inconsistent argument naming.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeTokenMinter.sol#L41

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VirtualBalanceRewardPool.sol#L190

---

[QA-3] Naming inconsistency at private state variables

Throughout the codebase, private state variables have _ at their prefixes. However, following private state variables do not follow this pattern.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/RewardFactory.sol#L18

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol#L33-L35

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L30

---

[QA-4][Migrations.sol] Naming inconsistency at state variable

Throughout the codebase, state variables which are not constant or private have camel case. However, following code is the exception.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Migrations.sol#L6

uint256 public last_completed_migration;

To follow the pattern at other files, it should be written as:

uint256 public lastCompletedMigration;

---

[QA-5] Incorrect comments

• updateable ---> updatable https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L64

• shutdown only sets true at isShutdown. It does not do unstake all tokens. release all locks https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L192

---

[QA-6] Use either msg.sender or _msgSender()

Following place uses _msgSender() although other functions in the same contract use msg.sender.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/RewardFactory.sol#L86 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L342

It should use either msg.sender or _msgSender() to be consistent.

---

[QA-7] Some libraries are not used

DepositToken sets following libraries. But they are not all used at all.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/DepositToken.sol#L11-L13

contract DepositToken is ERC20 {
    using SafeERC20 for IERC20;
    using Address for address;
    using SafeMath for uint256;

Other contracts seem to have similar situation.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/PoolManager.sol#L15-L17 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/TokenFactory.sol#L14

If they are not used, it should remove setting these libraries.

---

[QA-8][VeAssetDepositor.sol] Anyone can call deposit or depositAll with arbitrary _stakeAddress

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L127-L131

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L173

Following codes approve arbitrary _stakeAddress to spend _amount and do external call for _stakeAddress.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L162-L163

It is worth considering having a guard system to prevent attackers to set malicious _stakeAddress,

---

[QA-9] Unnecessary return variables

There are some functions specify return variables, but they are not used.

This is an example, and amount is not used.

    function lockedBalanceOf(address _user) external view returns (uint256 amount) {
        return balances[_user].locked;
    }

Following functions have this situation.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L295 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L300 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L332 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L376

---

[QA-10] Unnecessary return statement

Following functions define return variable, but it also have return statement. It can remove the return statement such as return balance;.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L291

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol#L119

---

[QA-11][VE3DLocker.sol] Inconsistent order of event definitions

Events are defined at the bottom of the code in VE3DLocker.sol.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L819-L830

At other files, it follows the style guide of solidity link.

Inside each contract, library or interface, use the following order:
1. Type declarations
2. State variables
3. Events
4. Modifiers
5. Functions

[GAS-1] Use custom errors

Using custom errors can reduce the gas cost.

There are more than 100 callsites of require check with inline error message. Using the custom errors instead of the inline error messages can reduce the gas cost.

---

[GAS-2] Use != 0 instead of > 0 on uint variables

uint variables will never be lower than 0. Therefore, > 0 and != 0 have same meanings. Using != 0 can reduce the gas deployment cost, so it is worth using != 0 wherever possible.

If the codebase uses > 0 on uint variables like this:

require(_amount > 0, "...");

it can rewrite like this:

require(_amount != 0, "...");

The above change can be applied to the following codes.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L173

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L215

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L273

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L517

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L526

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L541

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L551

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L556

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L562

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L586

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L590

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV1.sol#L111

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV1.sol#L156

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L68

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L106

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L219

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV3.sol#L74

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV3.sol#L132

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L180

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L340

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L520

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L626

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L649

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L670

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L679

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L781

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L210

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L234

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L285

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L89

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

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L132

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L138

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VirtualBalanceRewardPool.sol#L150

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol#L100

---

[GAS-3] No need to set 0 on uint variables

The default value of uint varibles are 0. Therefore, there is no need to set 0 on uint variables. Not setting 0 on uint variables can reduce the gas cost.

If 0 is set on uint variable like this uint256 amount = 0;, it can rewrite to uint256 amount;. In for loop, if uint is used like this for (uint256 i = 0; i < length; i++), it can omit setting = 0 and will be for (uint256 i; i < length; i++).

The above change can be applied to the following codes.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L66-L67

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L70-L72

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L176

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L218

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L245

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L282

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L329

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV1.sol#L29

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L71

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L78

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L137

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L181

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L213

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV3.sol#L84

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV3.sol#L126

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/RewardFactory.sol#L49

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/RewardFactory.sol#L66

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L286

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L420

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L425

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L613

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L803

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L148

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L214

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L238

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L257

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L281

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L326

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L28

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VirtualBalanceRewardPool.sol#L74-L75

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VirtualBalanceRewardPool.sol#L78-L80

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol#L217

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol#L227

---

[GAS-4] Potential usage of unchecked

Following variables or operations can be wrapped by unchecked.

• i++ (Why: extraRewards has upperbound extraRewards.length < EXTRA_REWARD_POOLS)

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L176

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L199

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L218

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L245

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L282

• i++ or x++ (Why: the end condition of for loop uses uint variable)

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L71

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L78

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/RewardFactory.sol#L49

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/RewardFactory.sol#L66

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L640

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L214

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L238

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L257

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L326

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L137

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV3.sol#L84

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L181

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L213

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV3.sol#L126

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L425

​​https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/ExtraRewardStashV2.sol#L140

---

[GAS-5] Usage of SafeMath

OZ's SafeMath library is used at various files.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L158-L160

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L180-L181

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L176-L181

These files only use basic functions of SafeMath library (add, sub, mul and div).

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/math/SafeMath.sol#L83-L137

Calling functions can increase the gas cost, and it is worth considering not using SafeMath library for basic operations.

---

[GAS-6][BaseRewardPool.sol] Refactoring opportunity

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L329-L336

This part can be written like this which results in reducing the gas cost.

        if (block.timestamp < periodFinish) {
            uint256 remaining = periodFinish.sub(block.timestamp);
            uint256 leftover = remaining.mul(rewardRate);
            reward = reward.add(leftover);
        }
        rewardRate = reward.div(duration);

---

[GAS-7][VE3DRewardPool.sol] Use _totalSupply private state variable instead of calling totalSupply() function

_totalSupply private state variable is defined at VE3DRewardPool contract.

Instead of calling totalSupply() at this code, it can simply access _totalSupply private state variable which results in gas cost reduction.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L171

uint256 supply = totalSupply();

---

[GAS-8] No need to set false on bool variable

The default value of bool type is false. It sets false at isShutdown state variable, but this is not needed.

These codes do not need to set false.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DLocker.sol#L106

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L110

---