veToken Finance contest - oyc_109's results

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L138-L143 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L274-L285

Vulnerability details

description

a compromised or malicious operator can withdraw all tokens by calling the function withdrawAll

/2022-05-vetoken/contracts/VoterProxy.sol
138:     function withdrawAll(address _token, address _gauge) external returns (bool) {
139:         require(msg.sender == operator, "!auth");
140:         uint256 amount = balanceOfPool(_gauge).add(IERC20(_token).balanceOf(address(this)));
141:         withdraw(_token, _gauge, amount);
142:         return true;
143:     }

the operator can also call an arbitary address with any value

/2022-05-vetoken/contracts/VoterProxy.sol
274:     function execute(
275:         address _to,
276:         uint256 _value,
277:         bytes calldata _data
278:     ) external returns (bool, bytes memory) {
279:         require(msg.sender == operator, "!auth");
280: 
281:         (bool success, bytes memory result) = _to.call{value: _value}(_data);
282:         require(success, "!success");
283: 
284:         return (success, result);
285:     }

No Transfer Ownership Pattern

description

The current feeManager transfer process involves the current feeManager calling setFeeManager(). If the nominated EOA account is not a valid account, or is address(0), it is entirely possible the feeManager may accidentally transfer ownership to an uncontrolled account, breaking all functions which require msg.sender == feeManager

Recommend considering implementing a two step process where the owner nominates an account and the nominated account needs to call an accept() function for the transfer of ownership to fully succeed. This ensures the nominated EOA account is a valid and active account.

findings

/2022-05-vetoken/contracts/VeAssetDepositor.sol
53:     function setFeeManager(address _feeManager) external {
54:         require(msg.sender == feeManager, "!auth");
55:         feeManager = _feeManager;
56:         emit FeeManagerUpdated(_feeManager);
57:     }

the setOwner function in Booster.sol also does not follow this pattern

/2022-05-vetoken/contracts/Booster.sol
123:     function setOwner(address _owner) external {
124:         require(msg.sender == owner, "!auth");
125:         owner = _owner;
126:         emit OwnerUpdated(_owner);
127:     }

/2022-05-vetoken/contracts/VoterProxy.sol
62:     function setOwner(address _owner) external {
63:         require(msg.sender == owner, "!auth");
64:         owner = _owner;
65:     }

multiply before divide

description

due to rounding errors, multiplication should be done befire division

findings

/2022-05-vetoken/contracts/VeAssetDepositor.sol
74: uint256 unlockInWeeks = (unlockAt / WEEK) * WEEK;

Modifier side-effects

description

Modifiers should only implement checks and not make state changes and external calls which violates the checks-effects-interactions pattern. These side-effects may go unnoticed by developers/auditors because the modifier code is typically far from the function implementation.

findings

/2022-05-vetoken/contracts/BaseRewardPool.sol
137:     modifier updateReward(address account) {
138:         rewardPerTokenStored = rewardPerToken();
139:         lastUpdateTime = lastTimeRewardApplicable();
140:         if (account != address(0)) {
141:             rewards[account] = earned(account);
142:             userRewardPerTokenPaid[account] = rewardPerTokenStored;
143:         }
144:         emit RewardUpdated(account, rewards[account], rewardPerTokenStored, lastUpdateTime);
145:         _;
146:     }

internal function names

description

internal function names should have an underscore, eg _notifyRewardAmount

/2022-05-vetoken/contracts/BaseRewardPool.sol
327: function notifyRewardAmount(uint256 reward) internal updateReward(address(0)) {

do not calculate constants

description

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

Consequences: each usage of a "constant" costs more gas on each access. Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

findings

/2022-05-vetoken/contracts/VeAssetDepositor.sol
18: uint256 private constant WEEK = 7 * 86400;

/2022-05-vetoken/contracts/BaseRewardPool.sol
59: uint256 constant BLOCKS_PER_YEAR = BLOCKS_PER_DAY * 365;

/2022-05-vetoken/contracts/VeTokenMinter.sol
15: uint256 public constant maxSupply = 30 * 1000000 * 1e18; //30mil

Don't Initialize Variables with Default Value

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

findings

/andy/2022-05-vetoken/contracts/VeAssetDepositor.sol
28: uint256 public incentiveVeAsset = 0;

/2022-05-vetoken/contracts/BaseRewardPool.sol
66:     uint256 public periodFinish = 0;
67:     uint256 public rewardRate = 0;
70:     uint256 public queuedRewards = 0;
71:     uint256 public currentRewards = 0;
72:     uint256 public historicalRewards = 0;

/2022-05-vetoken/contracts/VoterProxy.sol
227: uint256 _balance = 0;

custom errors

description

Consider using Custom Errors as they are more gas efficient while allowing developers to describe the error in detail using NatSpec.

for loop optimisations

description

Uninitialized variables are assigned with the types default value.

Explicitly initializing a variable with it's default value costs unnecesary gas.

Suggest not initializing the for loop counter to 0.

An array’s length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Suggest storing the array’s length in a variable before the for-loop, and use it instead:

++i costs less gas compared to i++

++i costs less gas compared to i++ for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration)

Suggest using ++i instead of i++ to increment the value of an uint variable.

Increments can be unchecked

In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline.

taking all of the above, the recommended format for gas savings is

for (uint256 i; i < numIterations;) {  
 // ...  
 unchecked { ++i; }  
}  

findings

/2022-05-vetoken/contracts/Booster.sol
329: for (uint256 i = 0; i < poolInfo.length; i++) {

/2022-05-vetoken/contracts/BaseRewardPool.sol
176: for (uint256 i = 0; i < extraRewards.length; i++) {
199: for (uint256 i = 0; i < extraRewards.length; i++) {

/2022-05-vetoken/contracts/VE3DRewardPool.sol
148: for (uint256 i = 0; i < rewardTokens.length(); i++) {
214: for (uint256 i = 0; i < length; i++) {
281: for (uint256 i = 0; i < rewardTokens.length(); i++) {

named returns and a return statement isn’t necessary

description

Removing unused named returns variables can reduce gas usage (MSTOREs/MLOADs) and improve code clarity. To save gas and improve code quality: consider using only one of those.

findings

/2022-05-vetoken/contracts/VoterProxy.sol
109: function withdraw(IERC20 _asset) external returns (uint256 balance) {
...
119: return balance;