veToken Finance contest - VAD37's results

Lines of code

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

Vulnerability details

VE3DRewardPool.sol is forked from cvxRewardPool.sol. Unlike the original every token address is immutable. The current implementation bundle all token on platform together into a single pool contract. This introduces new addReward() function to add new token to the pool instead of deploy new contract for every new token.

Impact

New improvement introduce new centralize issue with admin power limitation. Compromised admin can replace tokenDeposit address anytime to take control of user rewards or deposits flow into unknown address.

The lack of finality in addReward() should be addressed.

Proof of concept

• Assume admin key is lost. Attacker can replace veAssetDeposits and ve3Token address for all tokens inside the pool to attacker wallet contract.
• Then attacker can withdraw any user rewards by calling getReward(). VE3DRewardPool.sol will approve all reward and transfer to attacker contract that implement deposit and stakeFor function

Recommend mitigation steps

The addReward() function should only allow add token once.

    function addReward(
        address _rewardToken,
        address _veAssetDeposits,
        address _ve3TokenRewards,
        address _ve3Token
    ) external onlyOwner { 
        require(rewardTokens.add(_rewardToken),"token already added"); // EnumerableSet return false if address already exist
        rewardTokenInfo[_rewardToken].veAssetDeposits = _veAssetDeposits; //veAssetDepositor idle token
        rewardTokenInfo[_rewardToken].ve3TokenRewards = _ve3TokenRewards; // baseRewardPool
        rewardTokenInfo[_rewardToken].ve3Token = _ve3Token; //ve3Idle
    }

In case of pool migration or add wrong pool address. There should be separate function for DAO specifically to remove pool. Or better create its own pool for each token like Convex.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/1be2f03670e407908f175c08cf8cc0ce96c55baf/contracts/VeTokenMinter.sol#L77-L81

Vulnerability details

VeTokenMinter.sol is similar to Convex token

Admin suppose to mint 30M ve3Token as total supply and add them to VeTokenMinter contract and allow Booster to transfer some of these token away to rewards user for depositing LP into booster. User with larger amount of ve3Token can receive bigger portion of token from reward pool.

It doesn't make sense to have extra withdraw function belong to admin to take away booster staking token.

Impact

The veFinance class diagram from README.md does not include withdraw function. And consider the original fork does not have centralized money flow issue that admin can blatantly take away from user. The withdraw function seem quite odds place to be.

The centralized issue of what admin intended to do with ve3Token need to be addressed.

Proof of concept

• As long as owner can withdraw all ve3Token from VeTokenMinter contract, it is possible to have booster stop working due to lack of ve3Token to rewards user.
• Owner have access to virtual token, basically can claim all rewards staked by all users if ever used.

Recommend mitigation step

Provide document explain the limitation of admin and ownership on ve3Token.

Lines of code

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

Vulnerability details

earmarkFees() claim fee rewards and split it into lockFees rewards and stakerLockRewards reward pool. The original fork version does not split the fee, but current version did

Impact

Under condition, admin set wrong fee incentive amount, lockFeesIncentive + stakerLockFeesIncentive != FEE_DENOMINATOR. It is possible to have feeToken stuck inside Booster.sol or contract revert due to fee bigger than balance.

Proof of concept

• User call earmarkFees() to send asset rewards to reward pools.
• assume lockFeeIncentive is 90% and stakerLockFeeIncentive is 0%
• _balance send to the pool rewards is not 100% total asset balance. Some leftover token still stuck inside the pool.
• Other user will include this fee balance next time and compound it to queue rewards after.
• If admin set fee later to 100% and booster will transfer out stuck token. It will delay rewards to user, which leads to unfair distribution of small rewards amount.

Recommendation migitation step

setFeeInfo() should add require check to prevent splited fee not combined to 100%.

add require(_lockFeesIncentive + _stakerLockFeesIncentive == FEE_DENOMINATOR), "!fee" here would suffice.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VE3DRewardPool.sol#L296-L299 https://github.com/code-423n4/2022-05-vetoken/blob/1be2f03670e407908f175c08cf8cc0ce96c55baf/contracts/VeAssetDepositor.sol#L134-L152

Vulnerability details

Project veToken is supposed to be a generalized version of Convex for non-Curve token. There is only one contract for all rewards token in the platform.

All ve3Token rewards are bundled together inside ve3DLocker and ve3DRewardPool in a loop. Instead of having its own unique contract like VeAssetDepositer or VoterProxy for each token.

Impact

If one token has pausable transfer, user cannot claim rewards or withdraw if they have multiple rewards include that pause token.

Right now the project intends to support only 6 tokens, including Ribbon token which has pausable transfer controlled by Ribbon DAO.

Normally, this would not be an issue in Convex where only a few pools would be affected by single coin. Since, veAsset are bundled together into single reward pool, it becomes a major problem.

Proof of concept

• Token like Ribbon pause token transfer by DAO due to an unfortunate event.
• VE3DRewardPool try call getReward(), VeAssetDepositor try deposit token from earned rewards does not work anymore because IERC20.transfer is blocked. This effectively reverts current function if user have this token reward > 0.

Recommended mitigation step

It would be a better practice if we had a second getReward() function that accepts an array of token that we would like to interact with. It saves gas and only requires some extra work on frontend website. Instead of current implementation, withdraw all token bundles together.