veToken Finance contest - SecureZeroX's results

Lines of code

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

Vulnerability details

Impact

All ve projects have Max time, and if maxTime variable is greater than MAX_TIME of connected ve projects, VeAssetDepositor could not work.

Proof of Concept

In Curve fi, MAX_TIME is a 4 years. But in VeAssetDepositor.sol, there is no validation for maxTime If maxTime is greater than 4 years, the contract won’t be working. If maxTime is zero, _lockVeAsset() function won’t be working as well.

Tools Used

Manual

Recommended Mitigation Steps

Add validation for maxTime or get maxTime from connected ve project.

1. Emit events for sensitive variable updates.

It would be great to emit an event before or after updating sensitive variables to make easy to track updates by community.

VE3Token.sol/setOperator() VeTokenMinter.sol / addOperator() VeTokenMinter.sol / removeOperator() VE3DRewardPool.sol / addOperator() VE3DRewardPool.sol / removeOperator() RewardFactory.sol / addOperator() RewardFactory.sol / removeOperator() RewardFactory.sol / setAccess() StashFactory / addOperator() StashFactory / removeOperator()

Recommendation Emit events before or after updating sensitive varaibles.

2. OZ ERC20.safeApprove() function has been depreciated.

safeApprove() function of Openzeppelin’s ERC20 have been depreciated. https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2268 https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2219 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L287 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L288 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L301 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L305 https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/Booster.sol#L374

Recommendation: Use safeIncreaseAllowance or safeDecreaseAllowance functions instead.

3. It looks like maxTime of VeAssetDepositor should be public Now, the maxTime of VeAssetDepositor.sol has been declared as a private, but I think this should be public.

Recommendation: Check this variable again.

4. Emit wrong event in stakeFor function of VE3DRewardPool.sol contract.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VE3DRewardPool.sol#L249 In this line, the msg.sender is a VeAssetDepositor.sol contract or any other address which is different from user’s address.

Recommendation: Emit event with actual user’s address.

5. Some features of Booster.sol can be implemented by using OZ contracts.

• There is owner variable and setOwner function, this can be implemented by inheriting OpenZeppelin Ownable contract.
• There is isShutdown variable and shutdownSystem function, this can be implemented by inheriting OpenZeppelin Pausable contract.

6. setRewardContracts of Booster.sol function could emit event without changes. setRewardContracts set values only if lockRewards is address(0), but event is emitted per each function call.

So if we use subgraph or anything else, it could lead to track wrong data.

Recommendation: Emit RewardcontractsUpdated event inside condition.

7. Some contracts didn’t inherit interfaces correctly. ITokenFactory is an interface of TokenFactory contract. However TokenFactory did not inherit ITokenFactory. This lead unexpected issue – ex. Interface has function, but implementation contract did not implement it.

IRewardFactory and RewardFactory are same case.

Recommendation: Inherit interfaces, and override functions to avoid any unexpected issues in the future.

1. There are some unused libraries like Address.sol

The following smart contracts imported Address.sol library and using it for address, but this library is not used anywhere.

Booster.sol DepositToken.sol ExtraRewardStashV1.sol ExtraRewardStashV2.sol ExtraRewardStashV3.sol PoolManager.sol RewardFactory.sol StashFactory.sol TokenFactory.sol VeAssetDepositor.sol VoterProxy.sol VE3Token.sol

The following smart contracts is using SafeERC20 library without any usage. VE3Token.sol

Recommendation: Remove unused libraries to reduce size of contract.

2. It is not required to initialize zero value.

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L28 Setting zero value is not required and it will just cost gas.

Recommendation Remove zero initialization.

3. Revert function calls if condition does not meet.

The following functions will only waste gas if condition does not meet.

VeAssetDepositor.sol/setFees(): waste gas if _lockIncentive is greater than 30. VeAssetDepositor.sol/initialLock(): waste gas if veVeAsset is not zero. Booster.sol / setRewardContracts(): waste gas if lockRewards is not address(0)

Any variable won’t be updated after this function call, however this will waste gas.

Recommendation Revert tx if condition does not meet.

4. >= 0 operation does not need for uint variables.

Since uint variable is always greator or equal than 0, >=0 will be always true.

The following scripts have >=0 check. https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VeAssetDepositor.sol#L62

Recommendation: Remove >=0 condition check for uint variables.

5. Some variables can be declared as immutable.

The following variables are set in only constructor, so they can be declared as immutable.

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

Recommendation Make maxTime variable as a immutable.

6. VeTokenMinter is using ERC20 as an interface for veToken We can use IERC20 interface contract for veToken, But current code base requires to import whole ERC20 token contract which will increase contract size.

Recommendation: Use IERC20 instead of ERC20

7. Some functions can be declared as external The following functions can be declared as a external

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

Recommendation Use external keyword

8. Cache array length before loop to reduce gas cost Cache length of array and use it in the loop is a good solution to reduce gas. Other wise, it will always trying to load length from storage in every loop.

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]

Recommendation Cache length of array and use it in loop.

9. updateReward could be executed twice in withdraw function at VE3DRewardPool.sol and BaseRewardPool.sol

Both getReward and withdraw functions have updateReward modifier. So If claim param is true in withdraw function, updateReward modifier will be called again for getting reward. This will waste another gas.

Recommendation Make internal function to get reward without modifier, and call it in withdraw function.

10. historialRewards in rewardTokenInfo can be removed. historicalRewards was not used inside or outside of reward pool contract.

If this variable is used for only track, we can use subgraph instead.

Recommendation Remove historicalRewards variable.