code423n4 commented 2 years ago

Handle

Tomio

Vulnerability details

Impact

First of all, this is an address zero issue, however, this could lead to an imbalance between total supply circulating and the actual balance that was assigned to another user, in the https://github.com/code-423n4/2022-01-yield/blob/main/contracts/ConvexYieldWrapper.sol#L125, the wrap() receive 2 parameter which is to and from, if you assign the to address to collateralVault or zero address, the total supply is inflating, but it cant be accessible due to checking on https://github.com/code-423n4/2022-01-yield/blob/main/contracts/ConvexYieldWrapper.sol#L101 which will return 0 if the contract trying to access the balanceOf collateralVault or zero address, even though that address had something, and your ERC20 https://github.com/yieldprotocol/yield-utils-v2/blob/main/contracts/token/ERC20.sol didn't have the check to check if it's minting to collateralVault or zero address. This could make the calculation wrong because https://github.com/code-423n4/2022-01-yield/blob/main/contracts/ConvexStakingWrapper.sol#L222 would be relying on totalsupply.

Proof of Concept

https://github.com/code-423n4/2022-01-yield/blob/main/contracts/ConvexYieldWrapper.sol#L125

Tools Used

Recommended Mitigation Steps

alcueca commented 2 years ago

The return value of _getTotalSupply which is used for reward calculations uses _totalSupply, which returns the right value regardless of what _getDepositedBalance might return for any specific account.

GalloDaSballo commented 2 years ago

I have a lot of things to say about this finding

First of all, the warden could have done a better job at formatting it. Additionally, when aiming for a high (or medium) severity, having zero code and zero POC is a tough sell.

That said the warden did identify something valid, specifically that if wrap is called with a to == address(0) the shares will be minted to an address that can't use them.

That's correct.

On the other hand, for impact, there seems to be no impact at all, as while some shares may be effectively unusable, the finding shows no proof of an actual accounting error, nor how having a totalSupply that is different from circulatingSupply.

Ultimately, because of lack of information in the finding, I'm marking this invalid

code-423n4 / 2022-01-yield-findings