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
Rank: 6/43
Findings: 4
Award: $1,919.77
🌟 Selected for report: 0
🚀 Solo Findings: 0
https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/IndexLogic.sol#L31 https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/IndexLogic.sol#L96 https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/BaseIndex.sol#L43 https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/BaseIndex.sol#L59
According to the provided source code, the user must transfer the underlying asset to the contract prior to calling mint() or the index token before to calling burn(). If these two actions are performed on the difference block, it introduces the risk that someone could frontrun the transaction by calling mint() or burn() immediately after a user transfers tokens to the contract, so claiming over the user's funds.
Due to the fact that the source code provided is incomplete, it is unclear when and how the token is transferred to the contract. However, I chose to submit this issue in case the team is still unaware of it.
None
It is recommended that two actions must be executed in one transaction to eliminate the frontrun risk.
#0 - olivermehr
2022-05-02T20:30:36Z
Duplicate issue of #19
The transferFrom function of vToken.sol can be done without any user permissions or strict security checks, requires only the caller must has ORDERER_ROLE as the access control, exposing it to the centralize risk if an orderer is compromised or act maliciously.
transferFrom forward the call to _transfer() which just also forward the call to NAV.transfer()None
#0 - jn-lp
2022-05-11T14:33:28Z
duplicates #55
🌟 Selected for report: IllIllI
Also found by: 0v3rf10w, 0xDjango, 0xkatana, Dravee, Kenshin, Tadashi, TerrierLover, abhinavmir, defsec, ellahi, fatima_naz, foobar, gzeon, hyh, joestakey, kebabsec, kenta, minhquanym, oyc_109, rayn, robee, sseefried, xpriment626, z3s
86.1007 USDC - $86.10
Important or state changes function should emit events upon successful execution for off-chain tracking.
An event of calling critical functions should be generated for security and off-chain monitoring purposes.
Solidity compiler assigns zero value to any uninitalized state/local variables by default. It may cause unexpected behavior if the code does not validate for uninitialized variable, the AUM fee can be transfer to address zero, for instance.
Every state and local variable should be explicitly initialized, or implement a validation for zero values (such as address zero) to ensure that the function will be successfully executed when all variables are iniitialized.
Please note that ERC20 _mint() already has the address zero validation, the transaction will be reverted if the address zero is provided; so, the transaction will be reverted anyway if feePool is address zero in IndexLogic:mint() and InexLogic:burn() becasue of this line and this line. However, it is recommended to include an address zero validation in _chargeAUMFee() to ensure that when called directly, it is still reverted when an address zero is supplied.
address(0) as the burning address: https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/libraries/NAV.sol#L47address(0xdead) as the burning address: https://github.com/code-423n4/2022-04-phuture/blob/594459d0865fb6603ba388b53f3f01648f5bb6fb/contracts/IndexLogic.sol#L82The protocol uses difference address as the burning address to transfer token to. This may increase unnneccessary complexity to the contract on supply checking or lead to inconsistency logics.
It is recommended to use only one address as the burning address across the protocol.
There is no validation of array length to ensure that all arrays are the same size.
It is recommended to validate the array length of all input arrays before processing.
🌟 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
38.5445 USDC - $38.54
Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Source: https://blog.soliditylang.org/2021/04/21/custom-errors/
Consider using custom errors instead if the contract uses solidity version 0.8.4 or above.
Using a prefix increment ++i costs less gas than a suffix increment i++.
Use prefix increment rather than suffix increment.
According to the fact that EVM is a stack-based machine with 256-bits size per stack. When the value are read or written in contract storage, a full 256-bits are read or written; so, packing multiple smaller variables in one slot can save more gas from reading and writing.
uint32 private blockTimestampLast can be declared next to the address public immutable override asset1 pack both variable in the same storage.