veToken Finance contest - 0x1f8b's results

Judge has assessed an item in Issue #9 as Medium risk. The relevant finding follows: Centralized risk The operator address can mint arbitrary amount of tokens. In addition, operator can also burn tokens from third-party accounts.

If the private key of the owner or minter address is compromised, the attacker will be able to mint an unlimited amount of tokens, or burn from arbitrary addresses.

operator can mint and burn arbitrary tokens. I believe this is unnecessary and poses a serious centralization risk.

Recomendation:

Consider reduce centralization risks with timelock contracts. Affected source code:

VE3Token.sol#L26-L36 VeToken.sol#L23-L33 DepositToken.sol#L26-L36

Low

Avoid duplicate entries

When BaseRewardPool.addExtraReward is called, it is not checked that the address already exists in the extraRewards array. This would allow the token to be duplicated and affect the economics of the project.

Affected source code:

• BaseRewardPool.sol#L126

Use hardcoded values

It is not good practice to hardcode values, but if you are dealing with addresses much less, these can change between implementations, networks or projects, so it is convenient to remove these addresses from the source code.

Affected source code:

• SmartWalletWhitelist.sol#L34

Outdated compiler

The pragma version used are:

pragma solidity 0.8.7;
pragma solidity >=0.4.0;
pragma solidity >=0.4.22 <0.9.0;
pragma solidity >=0.5.0;

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

Lack of input checks

The following methods have a lack checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Affected source code:

address(0):

• StashFactory.sol#L27
• ExtraRewardStashV1.sol#L40-L43
• ExtraRewardStashV2.sol#L49-L53
• ExtraRewardStashV3.sol#L49-L53
• BaseRewardPool.sol#L103-L106
• VeAssetDepositor.sol#L45-L48
• VirtualBalanceRewardPool.sol#L95-L97
• VoterProxy.sol#L50-L55
• VoterProxy.sol#L80
• VeTokenMinter.sol#L27
• VeTokenMinter.sol#L41
• VE3DRewardPool.sol#L97-L99
• Booster.sol#L105-L108
• VE3DLocker.sol#L120
• Booster.sol#L123
• Booster.sol#L129
• Booster.sol#L135
• Booster.sol#L162
• Booster.sol#L168
• Booster.sol#L177
• Booster.sol#L244
• VE3DLocker.sol#L176-L177

Less than FEE_DENOMINATOR:

• Booster.sol#L196-L197

Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

owner:

• VE3Token.sol#L21
• VeToken.sol#L18
• TokenFactory.sol#L13
• StashFactory.sol#L15
• RewardFactory.sol#L14
• PoolManager.sol#L14
• VoterProxy.sol#L62
• VeTokenMinter.sol#L10
• VE3DRewardPool.sol#L53
• Booster.sol#L123

feeManager:

• VeAssetDepositor.sol#L53
• Booster.sol#L129

setPoolManager:

• Booster.sol#L135

Centralized risk

The operator address can mint arbitrary amount of tokens. In addition, operator can also burn tokens from third-party accounts.

If the private key of the owner or minter address is compromised, the attacker will be able to mint an unlimited amount of tokens, or burn from arbitrary addresses.

operator can mint and burn arbitrary tokens. I believe this is unnecessary and poses a serious centralization risk.

Recomendation:

• Consider reduce centralization risks with timelock contracts.

Affected source code:

• VE3Token.sol#L26-L36
• VeToken.sol#L23-L33
• DepositToken.sol#L26-L36

Owner can't be changed

Although the contract has an owner, this cannot be changed, so if you want to delegate or sell the project, it could not be done while keeping the same contracts.

Recomendation:

• Consider adding the ability to change these governors.

Affected source code:

operator can't be changed:

• DepositToken.sol#L23
• ExtraRewardStashV1.sol#L46
• ExtraRewardStashV2.sol#L50
• ExtraRewardStashV3.sol#L50

dao can't be changed:

• SmartWalletWhitelist.sol#L33

Non-Critical

Avoid errors with transferFrom

The following methods take all the user's balance and send it through transferFrom, this call may fail, since transferFrom extracts the balance from the previously approved allowance, it is better to use the user's allowance in order to avoid the unnecessary failure when both amounts are not the same. It's better to use allowance instead of balanceOf.

Affected source code:

• Booster.sol#L388-L389

Dead code

The following contracts are never used. It's an excellent idea to take them out to save gas and improve the codding style. The contract ForceSend is not used and it's very dangerous.

Affected source code:

• ForceSend.sol#L5

Use constants

Use constants for static computations 2**255:

• FixedPoint.sol#L59

Use constant instead of storage for totalCliffs = 1000;:

• VeTokenMinter.sol#L28

Cache static keccack256 bytes4(keccak256("set_rewards_receiver(address)")):

• Booster.sol#L488

++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--

Affected source code:

• RewardFactory.sol#L49
• RewardFactory.sol#L66
• ExtraRewardStashV2.sol#L71
• ExtraRewardStashV2.sol#L78
• ExtraRewardStashV2.sol#L137
• ExtraRewardStashV2.sol#L140
• ExtraRewardStashV2.sol#L181
• ExtraRewardStashV2.sol#L213
• ExtraRewardStashV3.sol#L84
• ExtraRewardStashV3.sol#L126
• BaseRewardPool.sol#L176
• BaseRewardPool.sol#L199
• BaseRewardPool.sol#L218
• BaseRewardPool.sol#L245
• BaseRewardPool.sol#L282
• VoterProxy.sol#L217
• VE3DRewardPool.sol#L148
• VE3DRewardPool.sol#L214
• VE3DRewardPool.sol#L238
• VE3DRewardPool.sol#L257
• VE3DRewardPool.sol#L281
• VE3DRewardPool.sol#L326
• Booster.sol#L329
• VE3DLocker.sol#L207
• VE3DLocker.sol#L286
• VE3DLocker.sol#L315
• VE3DLocker.sol#L360
• VE3DLocker.sol#L387
• VE3DLocker.sol#L406
• VE3DLocker.sol#L425
• VE3DLocker.sol#L457
• VE3DLocker.sol#L640
• VE3DLocker.sol#L720
• VE3DLocker.sol#L803

Reduce boolean comparison

It's compared a boolean value using == true or == false, instead of using the boolean value. NOT opcode, it's cheaper to use EQUAL or NOTEQUAL when the value it's false, or just the value without == true when it's true, because it will use less opcodes inside the VM.

Remove == true in:

• RewardFactory.sol#L40
• RewardFactory.sol#L57
• RewardFactory.sol#L103
• VoterProxy.sol#L70
• VoterProxy.sol#L110
• VoterProxy.sol#L113

Change == false by ! in:

• VoterProxy.sol#L93
• VoterProxy.sol#L96
• Booster.sol#L498

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next (require pragma upgrade).

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).

Affected source code:

• Migrations.sol#L9
• BitMath.sol#L8
• BitMath.sol#L45
• FixedPoint.sol#L85
• FixedPoint.sol#L105
• FixedPoint.sol#L131
• FixedPoint.sol#L149

Unused imports

There are certain contract imports that are not used and can be removed.

    using SafeERC20 for IERC20;
    using Address for address;
    using SafeMath for uint256;

Affected source code:

• VE3Token.sol#L11-L13
• VE3Token.sol#L4-L7
• DepositToken.sol#L11-L13
• DepositToken.sol#L4-L7
• PoolManager.sol#L15-L17
• PoolManager.sol#L12
• PoolManager.sol#L4-L8

Gas saving using immutable.

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

Affected source code:

• StashFactory.sol#L27
• DepositToken.sol#L23
• SmartWalletWhitelist.sol#L33
• ExtraRewardStashV1.sol#L45-L49
• ExtraRewardStashV2.sol#L48-L53
• ExtraRewardStashV3.sol#L49-L53
• BaseRewardPool.sol#L102-L106
• VeAssetDepositor.sol#L45-L48
• VeAssetDepositor.sol#L50
• VirtualBalanceRewardPool.sol#L95-L97
• VoterProxy.sol#L30
• VeTokenMinter.sol#L27
• VE3DRewardPool.sol#L97-L99

Changing to immutable the variables _name and _symbol, the methods name and symbol could be pure:

• VE3DLocker.sol#L116-L117

Delete optimization

Use delete instead of set to default value (false or 0)

Affected source code:

• SmartWalletWhitelist.sol#L57

Avoid unused returns

Having a method that always returns the same value is not correct in terms of consumption, if you want to modify a value, and the method will perform a revert in case of failure, it is not necessary to return a true, since it will never be false. It is less expensive not to return anything, and it also eliminates the need to double-check the returned value by the caller.

Affected source code:

• BaseRewardPool.sol#L192
• BaseRewardPool.sol#L291
• BaseRewardPool.sol#L300
• VoterProxy.sol#L88
• VoterProxy.sol#L105
• VoterProxy.sol#L135
• VoterProxy.sol#L142
• VoterProxy.sol#L155
• VoterProxy.sol#L163
• VoterProxy.sol#L169
• VoterProxy.sol#L175
• VoterProxy.sol#L259
• Booster.sol#L572
• Booster.sol#L594
• Booster.sol#L613

Use storage pointers or memory

Use storage keyword for save gas in order to cache a storage pointer.

The recurring use of the same register reading from storage is a task that can be easily optimized by using storage or memory, a clear example is access to rewardTokenInfo[_rewardToken] but it can be seen throughout all contracts.

Affected source code:

rewardTokenInfo[_rewardToken]:

• VE3DRewardPool.sol#L286-L318

rewardData[_rewardsToken]:

• VE3DLocker.sol#L154
• VE3DLocker.sol#L209-L223
• VE3DLocker.sol#L234-L240
• VE3DLocker.sol#L234-L240