veToken Finance contest - minhquanym's results

1. safeApprove may revert for non-zero to non-zero approvals

This issue impacts two contracts:

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

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L162

Impact

OpenZeppelin’s safeApprove reverts for non-0 to non-0 approvals. This is considered in a few places where safeApprove is performed twice with the first one for 0 and then for the desired allowance. However, there are uses of safeApprove where it is called only once for the desired allowance but it is not clear that the current allowance is guaranteed to be zero. In such cases, safeApprove may revert.

Proof of concept

It can happen for some reasons. For example, if reward contract has some kind of max stake amount, stakeFor() will not use up all approved amount => allowance is non-zero next time user deposit.

Tools Used

Manual Review

Recommended Mitigation Steps

Approve to 0 first then approve the desired amount

IERC20(token).safeApprove(rewardContract, 0);

2. earmarkFees() will not work if fee params is set incorrectly

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

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

Impact

In Booster.earmarkFees() function, _lockFeesIncentive and _stakerLockFeesIncentive is calculated using balance of Booster.

There is no check when fee manager set lockFeesIncentive and stakerLockFeesIncentive.

If fee params is set incorrectly (i.e lockFeesIncentive + stakerLockFeesIncentive != FEE_DENOMINATOR), there are 2 cases

1. lockFeesIncentive + stakerLockFeesIncentive > FEE_DENOMINATOR Total fee will bigger than balance of Booster => safeTransfer() revert.

2. lockFeesIncentive + stakerLockFeesIncentive < FEE_DENOMINATOR Some fee will be left in contract => Have to call earmarkFees() again and again to claim the rest.

Proof of concept

1. Balance of Booster is 1000000

lockFeesIncentive = 10000
stakerLockFeesIncentive = 10
FEE_DENOMINATOR = 10000

_lockFeesIncentive = balance * lockFeesIncentive / FEE_DENOMINATOR = 1000000
_stakerLockFeesIncentive = balance * stakerLockFeesIncentive / FEE_DENOMINATOR = 1000000 = 1000

Now, _lockFeesIncentive + _stakerLockFeesIncentive = 1001000 > balance. Booster doesn’t have enough fee token so safeTransfer will revert.

Tools Used

Manual Review

Recommended Mitigation Steps

Add check when when fee manager set fee params

require(lockFeesIncentive + stakerLockFeesIncentive == FEE_DENOMINATOR);

3. Inverse loop is implemented incorrectly

Impact

In inverse loop, stop condition is i + 1 != 0 but this condition will revert instead of normally stopping the for loop.

Index variable i is unsigned integer. So when i + 1 == 0, value of i should be -1 which is impossible (i is unsigned). Instead, it will revert because of underflow.

Proof of concept

Impacted codes

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L315

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L360

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L387

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L406

Tool Used

Manual Review

Recommended Mitigation Steps

Review and change the stop condition of inverse loops.

4. unlockTime is increased every week, implementation is not align with documentation

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeAssetDepositor.sol#L106

Impact

In VeAssetDepositor._lockVeAsset() function, the comment says "increase time too if over 2 week buffer", but actually it will be increased every week.

This will not caused any loss of money, but it will make users confuse, misunderstand and cost users more gas to execute TX.

Proof of concept

1. Alice deposited at timestamp 1648814400 (Friday, April 1, 2022 12:00:00 PM).

unlockAt = block.timestamp + MAXTIME
= 1648814400 + 125798400
= 1774612800

unlockInWeeks = (unlockAt/WEEK)*WEEK
= (1774612800 / 604800) * 604800
= 2934 * 604800
= 1774483200

Assume that unlockTime will be updated to unlockInWeeks: unlockTime = 1774483200 2. Bob deposited at timestamp 1649332800 (Thursday, April 7, 2022 12:00:00 PM) - Thursday next week.

unlockAt = block.timestamp + MAXTIME
= 1649332800 + 125798400
= 1775131200

unlockInWeeks = (unlockAt/WEEK)*WEEK
= (1775131200 / 604800) * 604800
= 2935 * 604800
= 1775088000

Now because unlockInWeeks.sub(unlockTime) = 604800 > 2, unlockTime will be updated even though it's less than a week (from Friday to Thursday next week)

Recommended Mitigation Steps

Should check if unlockInWeeks.sub(unlockTime) > 2*WEEK

5. Should remove SafeMath library because solidity version is 0.8

Details

Since you use solidity version 0.8, can just remove SafeMath library and replace add, sub, mul, div to normal operator +, -, *, / because all arithmetic operations will revert on underflow and overflow from solidity 0.8.

Proof of concept

https://docs.soliditylang.org/en/v0.8.13/080-breaking-changes.html

Recommended Mitigation Steps

Remove SafeMath library and update math operator.

1. Save gas by keeping track of nextEpochDate instead of reading from storage.

Details

In VE3DLocker._checkpointEpoch(), there is a while loop to fill epoch gaps until the next epoch date. Each iter, it reads from storage 4 times (2 in the while condition, 2 when calculate nextEpochDate). In fact, nextEpochDate can be kept track simply by adding rewardsDuration each epoch.

Proof of concept

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DLocker.sol#L496-L497

Recommended Mitigation Steps

Use variable nextEpochDate to cache like this

uint256 nextEpochDate = uint256(epochs[epochs.length - 1].date);
while (nextEpockDate != currentEpoch) {
  uint256 nextEpochDate = nextEpockDate.add(rewardsDuration);
  epochs.push(Epoch({ supply: 0, date: uint32(nextEpochDate) }));
}

2. Save gas by caching rewardPerTokenStored

Details

In VE3DRewardPool.updateReward(), rewardPerTokenStored is written then read from storage. It can be cached from rewardPerToken() to save gas

Proof of concept

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L150

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L157-L159

Recommended Mitigation Steps

Cache rewardPerTokenStored like this

uint256 _cached = rewardPerToken(_rewardToken);
rewardTokenInfo[_rewardToken].rewardPerTokenStored = _cached;
…
…
rewardTokenInfo[_rewardToken].userRewardPerTokenPaid[account] = _cached;

3. Should keep a storage variable to save gas

Details

In VE3DRewardPool.notifyRewardAmount() function, rewardTokenInfo[_rewardToken] is used repeatedly (9 times). Each time, it require some gas to calculate the storage address of rewardTokenInfo[_rewardToken] (keccak256).

Instead we can keep a storage variable, so it only needs to calculate the storage address once only.

Proof of concept

Impacted functions: https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L275

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L341

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L367

Reference from solidity docs https://docs.soliditylang.org/en/v0.8.13/internals/layout_in_storage.html#mappings-and-dynamic-arrays

Recommended Mitigation Steps

Keep storage variable to save gas.

RewardTokenInfo storage _tmp = rewardTokenInfo[_rewardToken];