Phuture Finance contest - TrungOre's results
Crypto index platform, that simplifies your investments through automated, themed index products.
General Information
Platform: Code4rena
Start Date: 19/04/2022
Pot Size: $30,000 USDC
Total HM: 10
Participants: 43
Period: 3 days
Judges: moose-code, JasoonS
Total Solo HM: 7
Id: 90
League: ETH
Findings Distribution
Researcher Performance
Rank: 2/43
Findings: 4
Award: $4,962.00
🌟 Selected for report: 2
🚀 Solo Findings: 2
Lines of code
https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/IndexLogic.sol#L48 https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/IndexLogic.sol#L31
Vulnerability details
Impact
User can lose their fund
Proof of Concept
When users want to mint an index token, users need to transfer their assets to address(vToken) first, then call the mint() function of IndexLogic.sol. If users make it into 2 transactions, miner can manipulate it/ After users transfer their token to address(vToken), miner can front-run, and call mint() before users call mint(), and of course, the index token will be minted to miner instead of users
==> User will lose their fund
Tools Used
manual review
Recommended Mitigation Steps
Implement a contract to help users execute 2 actions (transfer to vToken + mint) into 1 transaction (like router in uniswapV2)
#0 - olivermehr
2022-05-02T20:30:18Z
Duplicate issue of #19
Findings Information
🌟 Selected for report: TrungOre
Labels
Awards
2022.9048 USDC - $2,022.90
External Links
Lines of code
https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/ManagedIndexReweightingLogic.sol#L32 https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/interfaces/IIndexRegistry.sol#L19
Vulnerability details
Impact
The list of assets won't be changed after reweight because of reverted tx
Proof of Concept
require(_updatedAssets.length <= IIndexRegistry(registry).maxComponents())
when reweight is not true, because as in the doc,
maxComponent is the maximum assets for an index, but _updatedAssets also contain the assets that you want to remove. So the comparision make no sense
Tools Used
manual review
Recommended Mitigation Steps
Require assets.length() <= IIndexRegistry(registry).maxComponents() at the end of function instead
Findings Information
🌟 Selected for report: TrungOre
Lines of code
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)
#0 - jn-lp
2022-05-13T13:56:18Z
Such a case is considered impossible due to the fact that it can only work with a 0xdead address
#1 - moose-code
2022-05-24T13:40:23Z
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
#2 - JasoonS
2022-05-25T19:49:28Z
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.
Findings Information
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0xDjango, 0xNazgul, 0xkatana, Dravee, Kenshin, MaratCerby, Tadashi, TerrierLover, Tomio, TrungOre, defsec, ellahi, fatherOfBlocks, fatima_naz, gzeon, joestakey, kenta, minhquanym, oyc_109, rayn, rfa, robee, simon135, slywaters, windhustler, z3s
Awards
31.3783 USDC - $31.38
Labels
External Links
Use memory variable to store decimals()
Affected code
- https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/IndexLogic.sol#L78
- https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/IndexLogic.sol#L79
Use newWeight != 0 instead of newWeight > 0
Affected code
Use _totalSupply != 0 instead of _totalSupply > 0
Affected code
Use uint _balance = oldShares instead of uint _balance = _NAV.balanceOf[_account]
Affected code
Add condition (i != 0) to if block
Affected code
Proof of concept
- Because
maxCapitalizationis initialized with value_capitalization[0], so we don't need to check if_capitalizations[i] > maxCapitalizationifi == 0