veToken Finance contest - jonatascm's results

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L313 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L316

Vulnerability details

Impact

An issue in withdrawAll function, permits the pool shutdown and not retrieve the tokens.

Proof of concept

The issue is reproduced the following way:

1. The poolManager calls shutdownPool any the _pid value
2. If staker withdrawAll function try an error
3. The function continue the execution and shutdown the pool
4. The manager won't know if anything goes wrong and maybe will loss the tokens of that pool

Recommended Mitigation Steps

function shutdownPool(uint256 _pid) external returns (bool) {
	...
  //withdraw from gauge
+ try IStaker(staker).withdrawAll(pool.lptoken, pool.gauge) {} catch {
+		return false;
+	}
- try IStaker(staker).withdrawAll(pool.lptoken, pool.gauge) {} catch {}
  pool.shutdown = true;
  gaugeMap[pool.gauge] = false;
	...
	return true;
}

Tools Used

VSCode

[G-01] Reduce calls to storage in function updateveAssetWeight in VeTokenMinter contract

Target codebase

VeTokenMinter.sol#L43-45

Impact

Reducing one call to totalWeight variable will reduce gas used

Recommended Mitigation Steps

---

...
///@dev weight is 10**25 precision
function updateveAssetWeight(address veAssetOperator, uint256 newWeight) external onlyOwner {
    require(operators.contains(veAssetOperator), "not an veAsset operator");
+		totalWeight += newWight - veAssetWeights[veAssetOperator];
-   totalWeight -= veAssetWeights[veAssetOperator];
    veAssetWeights[veAssetOperator] = newWeight;
-   totalWeight += newWeight;
}
...

Tools Used

VSCode

---

[G-02] Declare variables outside of constructor in VeTokenMinter contract

Target codebase

VeTokenMinter.sol#L26

Impact

For gas optimization declare constants variables outside of constructor

Recommended Mitigation Steps

+ uint256 public totalCliffs = 1000;
+ uint256 public reductionPerCliff = maxSupply.div(totalCliffs);;
...
constructor(address veTokenAddress) {
	veToken = ERC20(veTokenAddress);
-	totalCliffs = 1000;
-	reductionPerCliff = maxSupply.div(totalCliffs);
}
...

Tools Used

VSCode

---

[G-03] First multiply and then divide to not lose precision

Target codebase

VeAssetDepositor.sol#L74

VeAssetDepositor.sol#L103

Impact

If you divide first then multiply you may lose precision, to not lose precision you need to multiply first and then divide

Recommended Mitigation Steps

function initialLock() external {
	...
    if (veVeAsset == 0) {
		    uint256 unlockAt = block.timestamp + maxTime;
				//@audit Possible miss calculation unlockInWeeks = unlockAt
+			  uint256 unlockInWeeks = (unlockAt * WEEK) / WEEK;
-       uint256 unlockInWeeks = (unlockAt / WEEK) * WEEK;
			...
    }
	...
}
...
function _lockVeAsset() internal {
    ...
    uint256 unlockAt = block.timestamp + maxTime;
		//@audit Possible miss calculation unlockInWeeks = unlockAt
+	  uint256 unlockInWeeks = (unlockAt * WEEK) / WEEK;
-   uint256 unlockInWeeks = (unlockAt / WEEK) * WEEK;
    ...
}

Tools Used

VSCode

---

[G-04] USE != 0 INSTEAD OF > 0

Target codebase

VeAssetDepositor.sol#L89

VeAssetDepositor.sol#L117

VeAssetDepositor.sol#L132

VeAssetDepositor.sol#L138

VE3DRewardPool.sol#L210

VE3DRewardPool.sol#L234

VE3DRewardPool.sol#L253

VE3DRewardPool.sol#L285

VoterProxy.sol#L100

Booster.sol#L517

Booster.sol#L526

Booster.sol#L541

Booster.sol#L551

Booster.sol#L556

Booster.sol#L562

Booster.sol#L586

Booster.sol#L590

BaseRewardPool.sol#L173

BaseRewardPool.sol#L196

BaseRewardPool.sol#L215

BaseRewardPool.sol#L273

Impact

For gas optimization use ≠ 0 instead of > 0

Recommended Mitigation Steps

...
uint256 x;
+ require(x != 0, "x!=0");
- require(x > 0, "x>0");
...

Tools Used

VSCode

---

[G-05] Optimizing for

Target codebase

Booster.sol#L329

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

Impact

For gas optimization in for loop you have 2 options:

1. Change the i++ to ++i
2. Or change to following code:

...
uint256[] array = [...]

+ uint256 length = array.length; 
+ for (uint i = 0; i < length;){
...
+   uncheck{++i;}
+ }
- for (uint256 i = 0; i < array.length; i++) {...}
 

Tools Used

VSCode

---

[G-06] Don't need to declare initial values = 0

Target codebase

BaseRewardPool.sol#L66-73

VeAssetDepositor.sol#L28

VoterProxy.sol#L227

Impact

To optimize you don't need to declare default initial values to variables, eg. bool isFalse = false uint256 x = 0

Recommended Mitigation Steps

...
//BaseRewardPool.sol
+ uint256 public periodFinish;
+ uint256 public rewardRate;
- uint256 public periodFinish = 0;
- uint256 public rewardRate = 0;
...
+ uint256 public queuedRewards;
+ uint256 public currentRewards;
+ uint256 public historicalRewards;
- uint256 public queuedRewards = 0;
- uint256 public currentRewards = 0;
- uint256 public historicalRewards = 0;
...

...
//VeAssetDepositor.sol
+ uint256 public incentiveVeAsset;
- uint256 public incentiveVeAsset = 0;
...

...
//VoterProxy.sol
+ uint256 _balance;
- uint256 _balance = 0;
...

Tools Used

VSCode

---

[G-07] Use delete instead of setting to default

Target codebase

BaseRewardPool.sol#L274

BaseRewardPool.sol#L307

BaseRewardPool.sol#L320

VE3DRewardPool.sol#L286

VE3DRewardPool.sol#L348

VE3DRewardPool.sol#L361

VeAssetDepositor.sol#L119

VeAssetDepositor.sol#L141

Impact

To reduce gas cost of a function you should use delete instead of setting to default, check the following code

Recommended Mitigation Steps

...
//BaseRewardPool.sol
//Change to delete
+ delete rewards[_account];
- rewards[_account] = 0;
...
//Change to delete
+ delete queuedRewards;
- queuedRewards = 0;;
...

Tools Used

VSCode

---

[G-08] Declare variable as immutable

Target codebase

BaseRewardPool.sol#L62-63

Impact

The variables operator ,rewardManager, pid, stakingToken and rewardToken are fixed, you can declare them as immutable for a gas optimization

Recommended Mitigation Steps

...
+ IERC20 public immutable rewardToken;
+ IERC20 public immutable stakingToken;
+ address public immutable operator;
+ address public immutable rewardManager;
+ uint256 public immutable pid;

- IERC20 public rewardToken;
- IERC20 public stakingToken;
- address public operator;
- address public rewardManager;
- uint256 public pid;
...

Tools Used

VSCode

---

[G-09] Create modifiers for require used multiple times

Target codebase

VoterProxy.sol#L63

VoterProxy.sol#L68

VoterProxy.sol#L78

VoterProxy.sol#L84

VoterProxy.sol#L92

VoterProxy.sol#L128

VoterProxy.sol#L139

VoterProxy.sol#L151

VoterProxy.sol#L159

VoterProxy.sol#L167

VoterProxy.sol#L173

VoterProxy.sol#L186

VoterProxy.sol#L211

VoterProxy.sol#L225

VoterProxy.sol#L257

VoterProxy.sol#L263

VoterProxy.sol#L279

Impact

More than 2 similar requires can be replaced by a modifier

Recommended Mitigation Steps

Create these modifier and replace in functions inside VoterProxy.sol

+  modifier isOwner() {
+   require(msg.sender == owner, "!auth");
+  }
...
+  modifier isOperator() {
+   require(msg.sender == operator, "!auth");
+  }
...
+  modifier isDepositor() {
+    require(msg.sender == depositor, "!auth");
+  }

Tools Used

VSCode