veToken Finance contest - Picodes's results

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/main/contracts/VoterProxy.sol

Vulnerability details

Impact

For some veAsset project (for example Angle’s gauges, gauge contracts are upgradable, so interfaces and underlying LP tokens are subject to change, blocking and freezing the system. Note that this is not hypothetic as it happened a few weeks ago: see this snapshot vote. Therefore, the system should be robust to a change in the pair gauge / token.

Note that is doable in the current setup for the veToken team to rescue the funds in such case, hence it is only a medium issue. You’d have to do as follow: a painful shutdown of the Booster (which would lead to an horrible situation where you’d have to preserve backwards compatibility for LPs to save their funds in the new Booster), an operator change in VoterProxy to be able to call execute.

Mitigation steps

To deal with upgradeable contracts, either the VoterProxy needs to be upgradable to deal with any situation that may arise, either you need to add upgradeable “intermediate” contracts between the staker and the gauge that could be changed to preserve the logic.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L270 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/contracts/VoterProxy.sol#L140

Vulnerability details

Impact

When calling withdrawAll, to compute the amount to withdraw, the contract checks the balance of gauge tokens and assume that 1 gauge token = 1 LP token by doing uint256 amount = balanceOfPool(_gauge).add(IERC20(_token).balanceOf(address(this)));. Overall this assumption may not hold and this would lead to a loss of funds when calling withdrawAll.

Proof of concept

Indeed this is false in some cases, check for example https://etherscan.io/address/0x3785Ce82be62a342052b9E5431e9D3a839cfB581 where the total supply is not the same as the balance of LP tokens held by the contract. You can also check the contract code where you can see there is a staking_factor between the balance and the underlying LP token balance.

Mitigation steps

Use the total supply of pool.token which is a better proxy to know how much to withdraw when withdrawing all.

Lines of code

https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/migrations/25_deploy_angle_pools.js#L68 https://github.com/code-423n4/2022-05-vetoken/blob/2d7cd1f6780a9bcc8387dea8fecfbd758462c152/migrations/25_deploy_angle_pools.js#L80

Vulnerability details

Impact

The address of G-Uni tokens in the deployment scripts are not up to date.

Proof of concept

For example for agEUR/USDC it is 0xedecb43233549c51cc3268b5de840239787ad56c and not 0x2bD9F7974Bc0E4Cb19B8813F8Be6034F3E772add

Mitigation steps

For safety why not fetching directly the LP token from the staking contract ?


[Non-Critical 01] - Typo

Here, it should be platform fees

[Non-Critical 02] - Why not using NatSpec and properly commenting functions ?

Most functions aren’t commented and comments do not follow the official specs, which could make sense as variable names are explicit, but recall that for non experienced dev, more and more explorers (see https://twitter.com/etherscan/status/1472191164807712768 ) are now supporting NatSpec so it’s worth adding explicit comments.

Reference: https://docs.soliditylang.org/en/v0.8.14/natspec-format.html

[Low 01] - setOperator cannot be called

In Ve3Token, there is a function to update the minter role, but this function cannot be called by VeAssetDepositor. This is to me a low severity issue as it may be intended that once linked to a Depositor the minter role cannot be updated anymore.

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