code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/vToken.sol#L160

Vulnerability details

Impact

Users can get the wrong amount of vToken => Make users lose their fund

Proof of Concept

Base on the code in function shareChange() in vToken.sol Assume that if oldShare = totalSupply > 0,

newShares = (_amountInAsset * (_totalSupply - oldShares)) / (_assetBalance - availableAssets); = (_amountInAsset * (_totalSupply - _totalSupply)) / (_assetBalance - availableAssets); = 0

It make no sense, because if amountInAsset >> availableAssets, newShares should be bigger than oldShares, but in this case newShares = 0 < oldShares

Tools Used

manual review

Recommended Mitigation Steps

Modify the line from if (_totalSupply > 0) to if (_totalSupply - oldShares > 0)

jn-lp commented 2 years ago

Such a case is considered impossible due to the fact that it can only work with a 0xdead address

moose-code commented 2 years ago

Agree its not an issue as on initialization tokens are sent to the burn address making this unlikely.

However the orderer role could possibly burn the tokens held by the burn address causing this issue to happen

JasoonS commented 2 years ago

Agree with mitigation step:

Modify the line from if (_totalSupply > 0) to if (_totalSupply - oldShares > 0)

If it were impossible for tokens to be burned from the 0xdead address then this wouldn't be a concern.

So although extremely unlikely, this is valid.

code-423n4 / 2022-04-phuture-findings