veToken Finance contest - catchup's results

Event missing indexed field

Each event can have up to 3 indexed fields. Destination address of the withdraw() function can be made indexed.

Lines of code

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

Use modifiers only for checks

The code inside a modifier is usually executed before the function body, so any state changes or external calls will violate the Checks-Effects-Interactions pattern. Moreover, these statements may also remain unnoticed by the developer, as the code for modifier may be far from the function declaration. https://consensys.net/blog/developers/solidity-best-practices-for-smart-contract-security/#:~:text=Use%20modifiers%20only%20for%20checks

Lines of code

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

Missing non-zero address checks when setting priviliged roles

It is a good practice to include non-zero address check especially when updating important addresses. I suggest to include non-zero address checks when setting addresses such as owner, fee manager, pool manager, etc.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L123 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L129 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L135 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L123 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol#L62 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol#L77

It would be better to make owner update as a two-step process

setOwner() is called by the current owner to update the owner address. It can be a better approach to follow a 2-step process when updating such priviliged addresses First transaction proposes the pending owner address, second transaction which can only be called by the proposed address accepts the role.

Lines of code

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

Missing events on onlyOwner operations

Functions that are only executable by privileged users (e.g. onlyOwner) and have an impact (e.g. financial, trust) on other users should emit events.

Lines of code

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/VeTokenMinter.sol#L32 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeTokenMinter.sol#L36 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L107 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L114 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L118

Missing non-zero address checks for token transfers

Tokens would be burned if sent to zero address accidentally. Therefore, it is a good practice to include non-zero address checks for token transfers.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L233 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeTokenMinter.sol#L48 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeTokenMinter.sol#L77

for loops can be optimized

1- For loop index increments can be made unchecked for solidity versions > 0.8.0. There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases. 2- Postfix increments can be changed as prefix as it is cheaper. 3- There is no need to set uint256 variable to 0 since default value is already 0.

Lines of code

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 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/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/VoterProxy.sol#L217

Recommended Mitigation Steps

I suggest to change the original code from this
        for (uint256 i = 0; i < extraRewards.length; i++) {
            IRewards(extraRewards[i]).stake(msg.sender, _amount);
        }

to this

        for (uint256 i; i < extraRewards.length; ) {
            IRewards(extraRewards[i]).stake(msg.sender, _amount);
	    unchecked { ++i; }
        }

Cache the state variable within for loops

The end indexes of the for loops are state variables and they are used directly. That means for every iteration an SLOAD needs to be performed. This is unnecessary consodering SLOAD is costly (~100 gas). It is much better to cache the length to a variable and use the data from stack.

Lines of code

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 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L329 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/VE3DRewardPool.sol#L148 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L281

Recommended Mitigation Steps

I suggest to change the original code from this

        for (uint256 i = 0; i < extraRewards.length; i++) {	//@audit SLOAD for every iteration
            IRewards(extraRewards[i]).stake(msg.sender, _amount);
        }

to this

	uint256 extraRewardsLength = extraRewards.length	//@audit SLOAD only once
        for (uint256 i; i < extraRewardsLength; ) {
            IRewards(extraRewards[i]).stake(msg.sender, _amount);
	    unchecked { ++i; }
        }

Redundant initialisation with default value

Some variables are initialised with their default values which cause unnecessary gas consumption

Lines of code

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/VeAssetDepositor.sol#L28 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol#L227

Using != 0 is cheaper than > 0 when used on a uint in a require() statement

Lines of code

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#L196 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/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#L253 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L132

Missing non-zero amount checks

It is a good practice to apply non-zero amount checks for token transactions to avoid unnecessary executions. This has been applied in most of the transactions, but still there are some missing instances.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L239 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/BaseRewardPool.sol#L294 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L428 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L442 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L336 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L169 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeTokenMinter.sol#L48 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeTokenMinter.sol#L77

Some public state variables can be immutable

Some public state variables are only set in the constructor and then never updated again. These can be changed to immutable.

Lines of code

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

Booleans are more expensive than uint256

It is more expensive to operate using booleans because before every write operation an SLOAD is executed to read the contents of the slot. Therefore, it is cheaper to use uint256 instead of bool. On the other hand, using bool is better for the code readability. Hence, it is a tradeoff to be decided by the developers. Another advantage of using uint over bool is that, SSTORE from 0 to a nonzero value costs 20000 gas, whereas SSTORE from nonzero to nonzero costs 5000 gas. Therefore, bool variables which toggle between 0/1 can be changed to uint and the values can alternate as 1/2.

This can be applied to variables such as isShutdown in Booster.sol.

Lines of code

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