veToken Finance contest - shenwilly's results

Judge has assessed an item in Issue #234 as Medium risk. The relevant finding follows:

1. Update asset weight when calling VeTokenMinter.removeOperator When removing operator, there is no check to make sure that veAssetWeights[operator] has been removed. If governance forgot to call updateveAssetWeight before removing the operator, totalWeight would be inflated and user would get lesser amount when claiming reward:

uint256 _veAssetEarned = _amount.mul(veTokenMinter.veAssetWeights(address(this))).div( veTokenMinter.totalWeight() ); Recommended Mitigation Ensure that weight is updated to zero when removing operator:

function removeOperator(address _operator) public onlyOwner { updateveAssetWeight(_operator, 0); // modify visibility to public operators.remove(_operator); } Relevant Links https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeTokenMinter.sol#L36-L46 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L607-L609

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/token/VeToken.sol#L30-L34

Vulnerability details

Impact

Governance can burn any amount of VeToken from any address.

Unlike VE3Token which is minted when users deposit veAsset and burned when users withdraw, the burn function in the governance token VeToken.sol is unnecessary and open up the risk of malicious/compromised governance burning user's token.

Recommended Mitigation Steps

Consider removing the function, or modify the burn function so it only allows msg.sender to burn the token:

function burn(uint256 _amount) external {
    _burn(msg.sender, _amount);
}

Low Risk Vulnerabilities

1. Update asset weight when calling VeTokenMinter.removeOperator

When removing operator, there is no check to make sure that veAssetWeights[operator] has been removed. If governance forgot to call updateveAssetWeight before removing the operator, totalWeight would be inflated and user would get lesser amount when claiming reward:

uint256 _veAssetEarned = _amount.mul(veTokenMinter.veAssetWeights(address(this))).div(
    veTokenMinter.totalWeight()
);

Recommended Mitigation

Ensure that weight is updated to zero when removing operator:

function removeOperator(address _operator) public onlyOwner {
    updateveAssetWeight(_operator, 0); // modify visibility to public
    operators.remove(_operator);
}

Relevant Links

• https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VeTokenMinter.sol#L36-L46
• https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/Booster.sol#L607-L609

2. Missing duplicate check in VE3DRewardPool.addReward

Duplicates in rewardTokens could cause irregular behaviour in the system.

Unfortunately we didn't have enough time to test whether a duplicate could break functionalities in the system (especially during VE3DRewardPool.getReward). Recommend to prevent the possibility of duplication as the duplicated element cannot be removed and it would cost more gas than necessary when iterating over rewardTokens.

Recommended Mitigation

Add a check to ensure _rewardToken isn't already added:

function addReward(
    address _rewardToken,
    address _veAssetDeposits,
    address _ve3TokenRewards,
    address _ve3Token
) external onlyOwner {
    rewardTokenInfo[_rewardToken].veAssetDeposits = _veAssetDeposits;
    rewardTokenInfo[_rewardToken].ve3TokenRewards = _ve3TokenRewards;
    rewardTokenInfo[_rewardToken].ve3Token = _ve3Token;
    
    if (!rewardTokens.contains(_rewardToken)) {
        rewardTokens.add(_rewardToken);
    }
}

Relevant Links

• https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L102-L112
• https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L281-L321

3. Depositors can supply arbitrary _stakeAddress

Malicious depositor could call VeAssetDepositor.deposit with a custom _stakeAddress contract, leading to unexpected behaviour.

function deposit(
    uint256 _amount,
    bool _lock,
    address _stakeAddress
) public {
    ...
    //mint here
    ITokenMinter(minter).mint(address(this), _amount);
    //stake for msg.sender
    IERC20(minter).safeApprove(_stakeAddress, _amount);
    IRewards(_stakeAddress).stakeFor(msg.sender, _amount);
    ...
}

For instance, someone could supply a contract that did nothing when stakeFor is called, leaving the token in the contract and unused allowance.

While it currently doesn't negatively impact the system, future changes on the code might not consider this and could potentially make a false assumption. In addition, analytics that track Deposited event might record wrong values.

Recommended Mitigation

Consider adding a token balance check before and after stakeFor to ensure token is actually staked, or alternatively, add a whitelist for _stakeAddress.

Relevant Links

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