QA Report

[code423n4]

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)) {

[GalloDaSballo]

No Transfer Ownership Pattern

NC

multiply before divide

Disagree, the division is done on purpose

Modifier side-effects

Disagree because the contract is SNX staking, the most battle tested contract in DeFi

internal function names

Valid NC

2NC