Back to reassor's contests

veToken Finance contest - reassor's results

Lock more veAsset permanently.

General Information

Platform: Code4rena

Start Date: 26/05/2022

Pot Size: $75,000 USDT

Total HM: 31

Participants: 71

Period: 7 days

Judge: GalloDaSballo

Total Solo HM: 18

Id: 126

League: ETH

veToken Finance

Findings Distribution

Researcher Performance

Rank: 28/71

Findings: 2

Award: $544.82

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryveToken Finance

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0x29A, 0xDjango, 0xNazgul, 0xf15ers, BouSalman, Chom, Deivitto, Dravee, ElKu, FSchmoede, Funen, GimelSec, Hawkeye, MiloTruck, Picodes, SecureZeroX, SmartSek, TerrierLover, WatchPug, _Adam, asutorufos, berndartmueller, c3phas, catchup, cccz, cogitoergosumsw, cryptphi, csanuragjain, delfin454000, dipp, ellahi, gzeon, hansfriese, horsefacts, hyh, kirk-baird, minhquanym, oyc_109, pauliax, reassor, robee, sashik_eth, shenwilly, simon135, sorrynotsorry, sseefried, unforgiven, xiaoming90, z3s

Awards

492.75 USDT - $492.75

Labels

bug
QA (Quality Assurance)

External Links

Link to GitHub issue

1. BaseRewardPool inaccurate number of days in a year calculation

Risk

Low

Impact

Contract BaseRewardPool defines BLOCKS_PER_YEAR that uses number of blocks a day multiplied by 365. This is inaccurate since for every year there is 365.25 days so the correct calcuation should be:

uint256 constant BLOCKS_PER_YEAR = BLOCKS_PER_DAY * 36525 / 100;

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to use following calculation:

uint256 constant BLOCKS_PER_YEAR = BLOCKS_PER_DAY * 36525 / 100;

2. Not following checks-effects-interactions pattern

Risk

Low

Impact

Function VeTokenMinter.mint does not follow checks-effects-interactions pattern which might lead to reentrancy attacks.

VeTokenMinter.sol:

(..)
veToken.safeTransfer(_to, _amount);
totalSupply += _amount;

Proof of Concept

VeTokenMinter.sol:

Tools Used

Manual Review / VSCode

It is recommended to first set the effects and then perform interactions such as external calls.

3. Missing zero address checks

Risk

Low

Impact

Multiple contracts do not check for zero addresses which might lead to loss of funds, failed transactions and can break the protocol functionality.

Proof of Concept

VeTokenMinter.sol:

VeAssetDepositor.sol:

VoterProxy.sol:

Booster.sol:

BaseRewardPool.sol:

VE3DRewardPool.sol:

Tools Used

Manual Review / VSCode

It is recommended to add zero address checks for listed parameters.

4. Missing events

Risk

Low

Impact

Multiple contracts are not implementing events for critical functions. Lack of events makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

VeTokenMinter.sol:

VoterProxy.sol:

Booster.sol:

VE3DRewardPool.sol:

Tools Used

Manual Review / VSCode

It is recommended to add missing events to listed functions.

5. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

VeTokenMinter.sol:

VeAssetDepositor.sol:

VoterProxy.sol:

Booster.sol:

VE3DRewardPool.sol:

Tools Used

Manual Review / VSCode

It is recommended to implement two-step process for changing critical addresses.

6. Lack of using native time units

Risk

Non-Critical

Impact

Contract VeAssetDepositor is using number of seconds in one day multiplied by 7 to define time units such as week. This might lead to confusion and accidental mistakes.

Proof of Concept

VeAssetDepositor.sol:18:    uint256 private constant WEEK = 7 * 86400;

Tools Used

Manual Review / VSCode

It is recommended to use native time units such as seconds, minutes, hours, days, weeks and years.

7. Use scientific notation

Risk

Non-Critical

Impact

Defining big numbers that consists of many digits might lead to accidential errors and mistakes.

Proof of Concept

VeTokenMinter.sol:

Tools Used

Manual Review / VSCode

It is recommended to use scientific notation.

8. Check return values

Risk

Non-Critical

Impact

Multiple contracts do not check for return values of executed functions. This might lead to accidents and errors since the created transaction is valid but the underlying code did not execute properly.

Proof of Concept

VeTokenMinter.sol:

VE3DRewardPool.sol:

Tools Used

Manual Review / VSCode

It is recommended to check the return vallues of executed functions.

9. Missing indexing for events

Risk

Non-Critical

Impact

Events should index addresses which helps off-chain applications in monitoring the protocol.

Proof of Concept

VeTokenMinter.sol:24:    event Withdraw(address destination, uint256 amount);

Tools Used

Manual Review / VSCode

It is recommended to add indexing to address type parameters.

10. Usage of boolean values in expressions

Risk

Non-Critical

Impact

Protocol uses boolean values in if and require expressions.

Proof of Concept

VoterProxy.sol:70:            operator == address(0) || IDeposit(operator).isShutdown() == true,
VoterProxy.sol:110:        require(stashPool[msg.sender] == true, "!auth");
VoterProxy.sol:113:        if (protectedTokens[address(_asset)] == true) {
Booster.sol:352:        require(pool.shutdown == false, "pool is closed");
Booster.sol:498:        require(pool.shutdown == false, "pool is closed");
VoterProxy.sol:93:        if (protectedTokens[_token] == false) {
VoterProxy.sol:96:        if (protectedTokens[_gauge] == false) {

Tools Used

Manual Review / VSCode

It is recommended to remove boolean values from if and require expressions.

11. Missing natspec comments

Risk

Non-Critical

Impact

Contracts are missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

  • BaseRewardPool.sol
  • Booster.sol
  • VE3DRewardPool.sol
  • VeAssetDepositor.sol
  • VeTokenMinter.sol
  • VoterProxy.sol

Tools Used

Manual Review / VSCode

It is recommended to add missing natspec comments.

#0 - GalloDaSballo

2022-07-08T00:44:57Z

1. BaseRewardPool inaccurate number of days in a year calculation

Marking as NC because the solution will round off

2. Not following checks-effects-interactions pattern

Valid Low

3. Missing zero address checks

Low

## 4. Missing events Valid NC

5. Critical address change

NC

## 6. Lack of using native time units Valid Refactoring

## 7. Use scientific notation R

8. Check return values

Because those functions give a guarantee of inclusion and return false only if the operation bear no changes, I think this one is invalid

9. Missing indexing for events

For that event, valid NC

10. Usage of boolean values in expressions

Valid R

11. Missing natspec comments

Valid NC

#1 - GalloDaSballo

2022-07-08T00:45:22Z

2L 3R 5NC

Findings Information

🌟 Selected for report: IllIllI

Also found by: 0x1f8b, 0x29A, 0xKitsune, 0xNazgul, 0xf15ers, 0xkatana, Cityscape, Dravee, ElKu, FSchmoede, Funen, GalloDaSballo, Hawkeye, Kaiziron, MiloTruck, Randyyy, RoiEvenHaim, Ruhum, SecureZeroX, SmartSek, TerrierLover, TomJ, Tomio, WatchPug, Waze, _Adam, asutorufos, c3phas, catchup, cogitoergosumsw, delfin454000, ellahi, fatherOfBlocks, gzeon, hansfriese, horsefacts, jonatascm, minhquanym, oyc_109, pauliax, reassor, robee, sach1r0, saian, sashik_eth, simon135, z3s

Awards

52.0655 USDT - $52.07

Labels

bug
G (Gas Optimization)

External Links

Link to GitHub issue

1. Obsolete check

Impact

Function VeAssetDepositor.setFees checks if _lockIncentive uint variable is bigger or equal to zero. That kind of check is obsolete since it returns true for all uint variables.

VeAssetDepositor.sol:62        if (_lockIncentive >= 0 && _lockIncentive <= 30) {

Proof of Concept

Tools Used

Manual Review / VSCode

It is recommended to just check if _lockIncentive <= 30.

2. Cache array length outside of loop

Impact

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Proof of Concept

Booster.sol:329:        for (uint256 i = 0; i < poolInfo.length; i++) {
BaseRewardPool.sol:176:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:199:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:218:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:245:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:282:            for (uint256 i = 0; i < extraRewards.length; i++) {
VE3DRewarDPool.sol:148:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewarDPool.sol:256:        uint256 length = extraRewards.length;
VE3DRewarDPool.sol:281:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VoterProxy.sol:217:            for (uint256 i = 0; i < _tokenVote.length; i++) {

Tools Used

Manual Review / VSCode

It is recommended to cache the array length outside of the for loop.

3. Use custom errors instead of revert strings to save gas

Impact

Usage of custom errors reduces the gas cost.

Proof of Concept

Contracts that could be using custom errors:

BaseRewardPool.sol:122:        require(msg.sender == rewardManager, "!authorized");
BaseRewardPool.sol:123:        require(_reward != address(0), "!reward setting");
BaseRewardPool.sol:124:        require(extraRewards.length < EXTRA_REWARD_POOLS, "!extra reward pools exceed");
BaseRewardPool.sol:132:        require(msg.sender == rewardManager, "!authorized");
BaseRewardPool.sol:173:        require(_amount > 0, "RewardPool : Cannot stake 0");
BaseRewardPool.sol:196:        require(_amount > 0, "RewardPool : Cannot stake 0");
BaseRewardPool.sol:215:        require(amount > 0, "RewardPool : Cannot withdraw 0");
BaseRewardPool.sol:301:        require(msg.sender == operator, "!authorized");
Booster.sol:124:        require(msg.sender == owner, "!auth");
Booster.sol:130:        require(msg.sender == feeManager, "!auth");
Booster.sol:136:        require(msg.sender == poolManager, "!auth");
Booster.sol:146:        require(msg.sender == owner, "!auth");
Booster.sol:163:        require(msg.sender == owner, "!auth");
Booster.sol:169:        require(msg.sender == voteDelegate, "!auth");
Booster.sol:179:        require(msg.sender == owner, "!auth");
Booster.sol:194:        require(msg.sender == feeManager, "!auth");
Booster.sol:226:        require(msg.sender == feeManager, "!auth");
Booster.sol:231:        require(total <= MaxFees, ">MaxFees");
Booster.sol:244:        require(msg.sender == feeManager, "!auth");
Booster.sol:261:        require(msg.sender == poolManager && !isShutdown, "!add");
Booster.sol:262:        require(_gauge != address(0) && _lptoken != address(0), "!param");
Booster.sol:309:        require(msg.sender == poolManager, "!auth");
Booster.sol:326:        require(msg.sender == owner, "!auth");
Booster.sol:350:        require(!isShutdown, "shutdown");
Booster.sol:352:        require(pool.shutdown == false, "pool is closed");
Booster.sol:360:        require(gauge != address(0), "!gauge setting");
Booster.sol:448:        require(msg.sender == rewardContract, "!auth");
Booster.sol:458:        require(msg.sender == voteDelegate, "!auth");
Booster.sol:468:        require(msg.sender == voteDelegate, "!auth");
Booster.sol:477:        require(msg.sender == stash, "!auth");
Booster.sol:485:        require(msg.sender == stash, "!auth");
Booster.sol:498:        require(pool.shutdown == false, "pool is closed");
Booster.sol:570:        require(!isShutdown, "shutdown");
Booster.sol:604:        require(msg.sender == rewardContract || msg.sender == lockRewards, "!auth")
VE3DRewardPool.sol:135:        require(msg.sender == rewardManager, "!authorized");
VE3DRewardPool.sol:136:        require(_reward != address(0), "!reward setting");
VE3DRewardPool.sol:142:        require(msg.sender == rewardManager, "!authorized");
VE3DRewardPool.sol:210:        require(_amount > 0, "RewardPool : Cannot stake 0");
VE3DRewardPool.sol:234:        require(_amount > 0, "RewardPool : Cannot stake 0");
VE3DRewardPool.sol:253:        require(_amount > 0, "RewardPool : Cannot withdraw 0");
VE3DRewardPool.sol:342:        require(operators.contains(_msgSender()), "!authorized");
VeAssetDepositor.sol:54:        require(msg.sender == feeManager, "!auth");
VeAssetDepositor.sol:60:        require(msg.sender == feeManager, "!auth");
VeAssetDepositor.sol:69:        require(msg.sender == feeManager, "!auth");
VeAssetDepositor.sol:132:        require(_amount > 0, "!>0");
VeTokenMinter.sol:42:        require(operators.contains(veAssetOperator), "not an veAsset operator");
VeTokenMinter.sol:49:        require(operators.contains(_msgSender()), "not an operator");
VoterProxy.sol:63:        require(msg.sender == owner, "!auth");
VoterProxy.sol:68:        require(msg.sender == owner, "!auth");
--
VoterProxy.sol:69:      require(
                         operator == address(0) || IDeposit(operator).isShutdown() == true,
                         "needs shutdown"
			);
--
VoterProxy.sol:78:        require(msg.sender == owner, "!auth");
VoterProxy.sol:84:        require(msg.sender == operator, "!auth");
VoterProxy.sol:92:        require(msg.sender == operator, "!auth");
VoterProxy.sol:110:        require(stashPool[msg.sender] == true, "!auth");
VoterProxy.sol:128:        require(msg.sender == operator, "!auth");
VoterProxy.sol:139:        require(msg.sender == operator, "!auth");
VoterProxy.sol:151:        require(msg.sender == depositor, "!auth");
VoterProxy.sol:159:        require(msg.sender == depositor, "!auth");
VoterProxy.sol:167:        require(msg.sender == depositor, "!auth");
VoterProxy.sol:173:        require(msg.sender == depositor, "!auth");
VoterProxy.sol:186:        require(msg.sender == operator, "!auth");
VoterProxy.sol:211:        require(msg.sender == operator, "!auth");
VoterProxy.sol:225:        require(msg.sender == operator, "!auth");
VoterProxy.sol:257:        require(msg.sender == operator, "!auth");
VoterProxy.sol:263:        require(msg.sender == operator, "!auth");
VoterProxy.sol:279:        require(msg.sender == operator, "!auth");
VoterProxy.sol:282:        require(success, "!success");

Tools Used

Manual Review / VSCode

It is recommended to add custom errors to listed contracts.

4. ++i/--i costs less gas compared to i++, i += 1, i-- or i -= 1

Impact

++i or --i costs less gas compared to i++, i += 1, i-- or i -= 1 for unsigned integer as pre-increment/pre-decrement is cheaper (about 5 gas per iteration).

Proof of Concept

BaseRewardPool.sol:176:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:199:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:218:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:245:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:282:            for (uint256 i = 0; i < extraRewards.length; i++) {
Booster.sol:329:        for (uint256 i = 0; i < poolInfo.length; i++) {
VE3DRewardPool.sol:148:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:214:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:238:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:257:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:281:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:326:            for (uint256 i = 0; i < length; i++) {
VoterProxy.sol:217:            for (uint256 i = 0; i < _tokenVote.length; i++) {

Tools Used

Manual Review / VSCode

It is recommended to use ++i or --i instead of i++, i += 1, i-- or i -= 1 to increment value of an unsigned integer variable.

5. Obsolete overflow/underflow check

Impact

Starting from solidity 0.8.0 there is built-in check for overflows/underflows. This mechanism automatically checks if the variable overflows or underflows and throws an error. Multiple contracts use increments that cannot overflow but consume additional gas for checks.

Proof of Concept

BaseRewardPool.sol:176:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:199:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:218:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:245:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:282:            for (uint256 i = 0; i < extraRewards.length; i++) {
Booster.sol:329:        for (uint256 i = 0; i < poolInfo.length; i++) {
VE3DRewardPool.sol:148:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:214:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:238:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:257:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:281:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:326:            for (uint256 i = 0; i < length; i++) {
VoterProxy.sol:217:            for (uint256 i = 0; i < _tokenVote.length; i++) {

Tools Used

Manual Review / VSCode

It is recommended wrap incrementing with unchecked block, for example: unchecked { ++i } or unchecked { --i }.

6. No need to explicitly initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for addresses). Explicitly initializing it with its default value is an anti-pattern and waste of gas.

Proof of Concept

BaseRewardPool.sol:66:    uint256 public periodFinish = 0;
BaseRewardPool.sol:67:    uint256 public rewardRate = 0;
BaseRewardPool.sol:70:    uint256 public queuedRewards = 0;
BaseRewardPool.sol:71:    uint256 public currentRewards = 0;
BaseRewardPool.sol:72:    uint256 public historicalRewards = 0;
BaseRewardPool.sol:176:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:199:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:218:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:245:        for (uint256 i = 0; i < extraRewards.length; i++) {
BaseRewardPool.sol:282:            for (uint256 i = 0; i < extraRewards.length; i++) {
Booster.sol:329:        for (uint256 i = 0; i < poolInfo.length; i++) {
VE3DRewardPool.sol:148:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:214:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:238:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:257:        for (uint256 i = 0; i < length; i++) {
VE3DRewardPool.sol:281:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
VE3DRewardPool.sol:326:            for (uint256 i = 0; i < length; i++) {
VeAssetDepositor.sol:28:    uint256 public incentiveVeAsset = 0;
VoterProxy.sol:217:            for (uint256 i = 0; i < _tokenVote.length; i++) {
VoterProxy.sol:227:        uint256 _balance = 0;

Tools Used

Manual Review / VSCode

It is recommended to remove explicit initializations with default values.

7. Use != 0 instead of > 0 for unsigned integer comparison

Impact

When dealing with unsigned integer types, comparisons with != 0 are cheaper than with > 0.

Proof of Concept

BaseRewardPool.sol:173:        require(_amount > 0, "RewardPool : Cannot stake 0");
BaseRewardPool.sol:196:        require(_amount > 0, "RewardPool : Cannot stake 0");
BaseRewardPool.sol:215:        require(amount > 0, "RewardPool : Cannot withdraw 0");
BaseRewardPool.sol:273:        if (reward > 0) {
Booster.sol:517:        if (veAssetBal > 0) {
Booster.sol:526:            if (treasury != address(0) && treasury != address(this) && platformFee > 0) {
Booster.sol:541:            if (_callIncentive > 0) {
Booster.sol:551:            if (_lockIncentive > 0) {
Booster.sol:556:            if (_stakerIncentive > 0) {
Booster.sol:562:            if (_stakerLockIncentive > 0) {
Booster.sol:586:        if (_lockFeesIncentive > 0) {
Booster.sol:590:        if (_stakerLockFeesIncentive > 0) {
VE3DRewardPool.sol:210:        require(_amount > 0, "RewardPool : Cannot stake 0");
VE3DRewardPool.sol:234:        require(_amount > 0, "RewardPool : Cannot stake 0");
VE3DRewardPool.sol:253:        require(_amount > 0, "RewardPool : Cannot withdraw 0");
VE3DRewardPool.sol:285:            if (reward > 0) {
VeAssetDepositor.sol:89:        if (veAssetBalance > 0) {
VeAssetDepositor.sol:117:        if (incentiveVeAsset > 0) {
VeAssetDepositor.sol:132:        require(_amount > 0, "!>0");
VeAssetDepositor.sol:138:            if (incentiveVeAsset > 0) {
VoterProxy.sol:100:        if (balance > 0) {

Tools Used

Manual Review / VSCode

It is recommended to use != 0 instead of > 0.

#0 - GalloDaSballo

2022-07-18T23:25:33Z

Saves less than 500 gas

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter