QA Report #264

code423n4 · 2022-06-02T19:43:51Z

Table of Contents

[L-01] Unsafe casting may overflow
[L-02] Check Effects Interactions pattern not respected
[L-03] Deprecated safeApprove() function
[L-04] Missing address(0) checks
[L-05] Unbounded loop on array can lead to DoS
[L-06] Lack of event emission
[L-07] Add a timelock to critical functions
[N-01] Missing friendly revert strings
[N-02] Unused named returns

[L-01] Unsafe casting may overflow

SafeMath and Solidity 0.8.* handles overflows for basic math operations but not for casting.
Consider using OpenZeppelin's SafeCast library to prevent unexpected overflows when casting from uint256 here:

contracts/VE3DLocker.sol:
  122          uint256 currentEpoch = block.timestamp.div(rewardsDuration).mul(rewardsDuration);
  123:         epochs.push(Epoch({supply: 0, date: uint32(currentEpoch)}));

  157  
  158:         rewardData[_rewardsToken].lastUpdateTime = uint40(block.timestamp);
  159:         rewardData[_rewardsToken].periodFinish = uint40(block.timestamp);

  500:                 epochs.push(Epoch({supply: 0, date: uint32(nextEpochDate)}));

  547:                 LockedBalance({amount: lockAmount, unlockTime: uint32(unlockTime)})

  585:                 userL.unlockTime = uint32(unlockTime);

  596:         e.supply = e.supply.add(uint224(lockAmount));

[L-02] Check Effects Interactions pattern not respected

To avoid unexpected behavior in the future (be it for the solution or for a fork), it's recommended to always follow the CEI pattern.

Consider always moving the state-changes before the external calls.

Affected code:

contracts/BaseRewardPool.sol:
  294      function donate(uint256 _amount) external {
  295          IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
  296:         queuedRewards = queuedRewards.add(_amount); //@audit : CEIP not respected. Move this line before the transfer
  297          emit Donated(queuedRewards);
  298      }

contracts/VE3DRewardPool.sol:
  336      function donate(address _rewardToken, uint256 _amount) external {
  337          IERC20(_rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
  338:         rewardTokenInfo[_rewardToken].queuedRewards += _amount;  //@audit : CEIP not respected. Move this line before the transfer
  339      }

contracts/VirtualBalanceRewardPool.sol:
  161      function donate(uint256 _amount) external {
  162          IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
  163:         queuedRewards = queuedRewards.add(_amount); //@audit : CEIP not respected. Move this line before the transfer
  164      }

[L-03] Deprecated safeApprove() function

Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (OpenZeppelin/openzeppelin-contracts#2219). The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

As recommended by the OpenZeppelin comment, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead:

contracts/Booster.sol:
  374:             IERC20(token).safeApprove(rewardContract, _amount); //@audit medium-risk: should do like everywhere in the solution and approve 0 first

contracts/VE3DLocker.sol:
  211:                 IERC20(rewardData[_rewardsToken].ve3Token).safeApprove(
  215:                 IERC20(rewardData[_rewardsToken].ve3Token).safeApprove(
  221:                 IERC20(_rewardsToken).safeApprove(rewardData[_rewardsToken].veAssetDeposits, 0);
  222:                 IERC20(_rewardsToken).safeApprove(

contracts/VE3DRewardPool.sol:
  287:                 IERC20(_rewardToken).safeApprove(rewardTokenInfo[_rewardToken].veAssetDeposits, 0);
  288:                 IERC20(_rewardToken).safeApprove(
  301:                     IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
  305:                     IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(

contracts/VeAssetDepositor.sol:
  162:             IERC20(minter).safeApprove(_stakeAddress, _amount);

contracts/VoterProxy.sol:
  101:             IERC20(_token).safeApprove(_gauge, 0);
  102:             IERC20(_token).safeApprove(_gauge, balance);
  152:         IERC20(veAsset).safeApprove(escrow, 0);
  153:         IERC20(veAsset).safeApprove(escrow, _value);
  160:         IERC20(veAsset).safeApprove(escrow, 0);
  161:         IERC20(veAsset).safeApprove(escrow, _value);

[L-04] Missing address(0) checks

Affected code (from Slither):

    - operator = _operator (token/VE3Token.sol#23)
    - operator = op_ (VirtualBalanceRewardPool.sol#97)
    - staker = _staker (VeAssetDepositor.sol#45)
    - minter = _minter (VeAssetDepositor.sol#46)
    - veAsset = _veAsset (VeAssetDepositor.sol#47)
    - escrow = _escrow (VeAssetDepositor.sol#48)
    - feeManager = _feeManager (VeAssetDepositor.sol#55)
    - rewardManager = rewardManager_ (VE3DRewardPool.sol#99)
    - staker = _staker (Booster.sol#111)
    - minter = _minter (Booster.sol#116)
    - veAsset = _veAsset (Booster.sol#117)
    - feeDistro = _feeDistro (Booster.sol#118)
    - owner = _owner (Booster.sol#125)
    - feeManager = _feeM (Booster.sol#131)
    - poolManager = _poolM (Booster.sol#137)
    - rewardFactory = _rfactory (Booster.sol#152)
    - tokenFactory = _tfactory (Booster.sol#153)
    - stashFactory = _sfactory (Booster.sol#159)
    - rewardArbitrator = _arb (Booster.sol#164)
    - voteDelegate = _voteDelegate (Booster.sol#170)
    - lockRewards = _rewards (Booster.sol#184)
    - stakerRewards = _stakerRewards (Booster.sol#185)
    - stakerLockRewards = _stakerLockRewards (Booster.sol#186)
    - treasury = _treasury (Booster.sol#245)
    - dao = _dao (helper/SmartWalletWhitelist.sol#33)
    - future_checker = _checker (helper/SmartWalletWhitelist.sol#40)
    - veAsset = _veAsset (ExtraRewardStashV3.sol#49)
    - operator = _operator (ExtraRewardStashV3.sol#50)
    - staker = _staker (ExtraRewardStashV3.sol#51)
    - gauge = _gauge (ExtraRewardStashV3.sol#52)
    - rewardFactory = _rFactory (ExtraRewardStashV3.sol#53)
    - veAsset = _veAsset (ExtraRewardStashV2.sol#49)
    - operator = _operator (ExtraRewardStashV2.sol#50)
    - staker = _staker (ExtraRewardStashV2.sol#51)
    - gauge = _gauge (ExtraRewardStashV2.sol#52)
    - rewardFactory = _rFactory (ExtraRewardStashV2.sol#53)
    - operator = _operator (token/VeToken.sol#20)
    - operator = _operator (ExtraRewardStashV1.sol#46)
    - staker = _staker (ExtraRewardStashV1.sol#47)
    - gauge = _gauge (ExtraRewardStashV1.sol#48)
    - rewardFactory = _rFactory (ExtraRewardStashV1.sol#49)
    - veAsset = _veAsset (VoterProxy.sol#50)
    - escrow = _escrow (VoterProxy.sol#51)
    - gaugeProxy = _gaugeProxy (VoterProxy.sol#52)
    - minter = _minter (VoterProxy.sol#54)
    - owner = _owner (VoterProxy.sol#64)
    - operator = _operator (VoterProxy.sol#74)
    - depositor = _depositor (VoterProxy.sol#80)
    - (success,result) = _to.call{value: _value}(_data) (VoterProxy.sol#281)
    - rewardFactory = _rewardFactory (StashFactory.sol#27)
    - operator = operator_ (BaseRewardPool.sol#105)
    - rewardManager = rewardManager_ (BaseRewardPool.sol#106)
    - operator = _operator (DepositToken.sol#23)

[L-05] Unbounded loop on array can lead to DoS

As these arrays can grow quite large (only push operations, no pop), the transaction's gas cost could exceed the block gas limit and make it impossible to call the impacted functions at all.

contracts/BaseRewardPool.sol:
  126:         extraRewards.push(_reward);
  176:         for (uint256 i = 0; i < extraRewards.length; i++) {
  199:         for (uint256 i = 0; i < extraRewards.length; i++) {
  218:         for (uint256 i = 0; i < extraRewards.length; i++) {
  245:         for (uint256 i = 0; i < extraRewards.length; i++) {
  282:             for (uint256 i = 0; i < extraRewards.length; i++) {

contracts/Booster.sol:
  282:         poolInfo.push(
  329:         for (uint256 i = 0; i < poolInfo.length; i++) {

contracts/RewardFactory.sol:
  49:         for (uint256 i = 0; i < length; i++) {
  52:         activeList.push(pid);
  66:         for (uint256 i = 0; i < length; i++) {
  71:                 activeList.pop();

contracts/VE3DLocker.sol:
  123:         epochs.push(Epoch({supply: 0, date: uint32(currentEpoch)}));
  156:         rewardTokens.push(_rewardsToken);
  207:         for (uint256 i; i < rewardTokens.length; i++) {
  286:         for (uint256 i = 0; i < userRewards.length; i++) {
  457:         for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
  500:                 epochs.push(Epoch({supply: 0, date: uint32(nextEpochDate)}));
  546:             userLocks[_account].push(
  579:                 userLocks[_account].push(
  640:             for (uint256 i = nextUnlockIndex; i < length; i++) {
  720:         for (uint256 i; i < rewardTokens.length; i++) {
  803:             for (uint256 i = 0; i < rewardTokens.length; i++) {

contracts/VE3DRewardPool.sol:
  138:         extraRewards.push(_reward);
  148:         for (uint256 i = 0; i < rewardTokens.length(); i++) {
  214:         for (uint256 i = 0; i < length; i++) {
  238:         for (uint256 i = 0; i < length; i++) {
  257:         for (uint256 i = 0; i < length; i++) {
  281:         for (uint256 i = 0; i < rewardTokens.length(); i++) {
  326:             for (uint256 i = 0; i < length; i++) {

Consider introducing a reasonable upper limit based on block gas limits and adding a method to remove elements in the array.

[L-06] Lack of event emission

According to Slither, these should emit events (indeed, state variables are being updated):

VirtualBalanceRewardPool.queueNewRewards(uint256) (VirtualBalanceRewardPool.sol#166-188) should emit an event for: 
 - queuedRewards = 0 (VirtualBalanceRewardPool.sol#173) 
 - queuedRewards = 0 (VirtualBalanceRewardPool.sol#184) 
 - queuedRewards = _rewards (VirtualBalanceRewardPool.sol#186) 
Booster.setFeeInfo(uint256,uint256) (Booster.sol#193-217) should emit an event for: 
 - lockFeesIncentive = _lockFeesIncentive (Booster.sol#196) 
 - stakerLockFeesIncentive = _stakerLockFeesIncentive (Booster.sol#197) 
VE3DLocker.setKickIncentive(uint256,uint256) (VE3DLocker.sol#185-190) should emit an event for: 
 - kickRewardPerEpoch = _rate (VE3DLocker.sol#188) 
BaseRewardPool.queueNewRewards(uint256) (BaseRewardPool.sol#300-325) should emit an event for: 
 - queuedRewards = 0 (BaseRewardPool.sol#307) 
 - queuedRewards = 0 (BaseRewardPool.sol#320) 
 - queuedRewards = _rewards (BaseRewardPool.sol#322)

[L-07] Add a timelock to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Consider adding a timelock to:

contracts/Booster.sol:
  219:     function setFees(

contracts/VeAssetDepositor.sol:
  59:     function setFees(uint256 _lockIncentive) external {

[N-01] Missing friendly revert strings

Here, a friendly message should exist for users to understand what went wrong:

contracts/VE3DLocker.sol:
  154:         require(rewardData[_rewardsToken].lastUpdateTime == 0);
  155:         require(_rewardsToken != address(stakingToken));
  165:             require(_ve3Token != address(0));
  166:             require(_ve3TokenStaking != address(0));
  167:             require(_veAssetDeposits != address(0));
  180:         require(rewardData[_rewardsToken].lastUpdateTime > 0);

[N-02] Unused named returns

While not consuming more gas with the Optimizer enabled: using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity:

contracts/VE3DLocker.sol:
  282:         returns (EarnedData[] memory userRewards)
  295:     function lockedBalanceOf(address _user) external view returns (uint256 amount) {
  300:     function balanceOf(address _user) external view returns (uint256 amount) {
  305:     function balanceAtEpochOf(uint256 _epoch, address _user) public view returns (uint256 amount) {
  332:     function pendingLockOf(address _user) external view returns (uint256 amount) {
  376:     function totalSupply() external view returns (uint256 supply) {
  399:     function totalSupplyAtEpoch(uint256 _epoch) external view returns (uint256 supply) {
  418:     function findEpochId(uint256 _time) public view returns (uint256 epoch) {

contracts/VoterProxy.sol:
  109:     function withdraw(IERC20 _asset) external returns (uint256 balance) {

The text was updated successfully, but these errors were encountered:

GalloDaSballo · 2022-07-07T22:35:51Z

[L-01] Unsafe casting may overflow

Valid Low

[L-02] Check Effects Interactions pattern not respected

Valid Low

[L-03] Deprecated safeApprove() function

Valid NC because the code is using approve as intended

## [L-04] Missing address(0) checks
Valid Low

[L-05] Unbounded loop on array can lead to DoS

Valid Low (May get bumped to Med)

[L-06] Lack of event emission

Valid NC

[L-07] Add a timelock to critical functions

Cannot be proven nor falsified, hence invalid

[N-01] Missing friendly revert strings

Valid NC

[N-02] Unused named returns

Valid Refactor

Nice report, with good useful findings

4L, 1R, 3NC

GalloDaSballo · 2022-07-28T17:29:54Z

TODO - Raise
L-05 -> M-07

GalloDaSballo · 2022-07-28T17:30:05Z

Updated score:
3L, 1R, 3NC

code423n4 added bug Something isn't working QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax labels Jun 2, 2022

code423n4 added a commit that referenced this issue Jun 2, 2022

Dravee issue #264

6e1e330

code423n4 mentioned this issue Jul 28, 2022

Upgraded Q -> M from 264 [1659038303960] #276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QA Report #264

QA Report #264

code423n4 commented Jun 2, 2022

GalloDaSballo commented Jul 7, 2022

GalloDaSballo commented Jul 28, 2022

GalloDaSballo commented Jul 28, 2022

QA Report #264

QA Report #264

Comments

code423n4 commented Jun 2, 2022

[L-01] Unsafe casting may overflow

[L-02] Check Effects Interactions pattern not respected

[L-03] Deprecated safeApprove() function

[L-04] Missing address(0) checks

[L-05] Unbounded loop on array can lead to DoS

[L-06] Lack of event emission

[L-07] Add a timelock to critical functions

[N-01] Missing friendly revert strings

[N-02] Unused named returns

GalloDaSballo commented Jul 7, 2022

[L-01] Unsafe casting may overflow

[L-02] Check Effects Interactions pattern not respected

[L-03] Deprecated safeApprove() function

[L-05] Unbounded loop on array can lead to DoS

[L-06] Lack of event emission

[L-07] Add a timelock to critical functions

[N-01] Missing friendly revert strings

[N-02] Unused named returns

GalloDaSballo commented Jul 28, 2022

GalloDaSballo commented Jul 28, 2022