Platform: Code4rena
Start Date: 07/04/2022
Pot Size: $100,000 USDC
Total HM: 20
Participants: 62
Period: 7 days
Judge: LSDan
Total Solo HM: 11
Id: 107
League: ETH
Rank: 20/62
Findings: 3
Award: $671.20
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Wayne
Also found by: Cr4ckM3, PPrieditis, hyh, rayn, smiling_heretic
https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L87
noContract() NatSpec description is "Modifier that ensures that non-whitelisted contracts can't interact with the LP farm". It is already stated that "some contracts will be able to bypass this check" however the impact is miscalculated and necessary gas to bypass the check is underestimated. Probably author has not taken it to an account that it is possible to create a contract, delete it and create a new contract with the same address.
Due to Create2() and selfdestruct() it is very easy and cheap to bypass the check and use the same contract address.
As an example we can look at Sherlock CTF: Challange: https://github.com/sherlock-protocol/sherlock-ctf-0x0/blob/master/teryanarmen/contracts/Challenge2.sol Solution, exploit: https://github.com/sherlock-protocol/sherlock-ctf-0x0/blob/master/teryanarmen/contracts/Exploit.sol Challange shows that it is possible to re-create multiple times contract with the same address.
Change modifier noContract() expression: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L87
From: !_account.isContract() || whitelistedContracts[_account] To: msg.sender == tx.origin || whitelistedContracts[_account]
Also remove import of "@openzeppelin/contracts/utils/Address.sol" because isContract() was the only function that was used from Address.sol.
Change is also needed for: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol
#0 - spaghettieth
2022-04-12T19:50:51Z
Duplicate of #11
🌟 Selected for report: Dravee
Also found by: 0x1f8b, 0xDjango, 0xkatana, AuditsAreUS, Cityscape, Foundation, Funen, Hawkeye, IllIllI, JC, JMukesh, Jujic, Kthere, PPrieditis, Picodes, Ruhum, TerrierLover, TrungOre, WatchPug, berndartmueller, catchup, cccz, cmichel, delfin454000, dy, ellahi, hickuphh3, horsefacts, hubble, hyh, ilan, jayjonah8, kebabsec, kenta, minhquanym, pauliax, rayn, reassor, rfa, robee, samruna
357.5167 USDC - $357.52
Jpeg tokens will be stuck if JPEGLock.lockFor() is called again before claiming tokens via function JPEGLock.unlock().
Change the locking logic. If there is already locked position then allow to increase, extend it, or throw a error: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L49-L63
remainingRewards will have the wrong value because it calculates how much rewards there should be in theory however in reality it will be a different number. Rewards will change due to rounding by claiming rewards or if pool.allocPoint() update call is done during a epoch. https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L119-L120
Don't calculate how much remaining rewards there should be in theory and use function balanceOf() to get how much rewards there are left in reality.
totalDebtAmount and totalFeeCollected will be wrong if accrue() is not called before setDebtInterestApr(). setDebtInterestApr() changes interest rate APR. If setDebtInterestApr() is called before accrue() then accrue() will use already the new rate andnot the old rate.
Add a call to accrue() in function setDebtInterestApr(): https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L212
Interest rate change has a direct impact on active loans via function function NFTVault_calculateAdditionalInterest()
Emit an event if NFTVault.setDebtInterestApr() is called and interest rate is changed: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L212
Multiplication before division decreases rounding errors.
Move multiplication before division at: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L369 and https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L595
Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the pragma (for e.g. by not using ^ in pragma solidity 0.5.10) ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs. More information can be seen in SWC Registry: https://swcregistry.io/docs/SWC-103
Use locked pragma for contracts.
Contracts inheriting from multiple contracts with identical functions should specify the correct inheritance order i.e. more general to more specific to avoid inheriting the incorrect function implementation. https://swcregistry.io/docs/SWC-125
Remove unnecessary imports. In the following files there are contract imports that aren't used: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/escrow/NFTEscrow.sol#L4
There are many instances of the value 1e36. Currently this integer value is used directly without a clear indication that this value relates to the decimals value, which could lead to one of these values being modified but not the other (perhaps by a typo), which is the basis for many past hacks. Coding best practices suggests using a constant integer to store this value in a way that clearly explains the purpose of this value to prevent confusion.
Create a new constant, for example, ROUNDING_PRECISION_MULTIPLIER
Change: Remove magic number and add a constant to: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol
Owner can change LP Farming rewards without any warning and history data of epoch like startBlock, endBlock, rewardPerBlock isn't stored anywhere.
Emit an event when LP Farming epoch data is changed in function LPFarming.newEpoch(): https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L107
🌟 Selected for report: Dravee
Also found by: 0v3rf10w, 0x1f8b, 0xDjango, 0xNazgul, 0xkatana, Cityscape, Cr4ckM3, FSchmoede, Foundation, Funen, Hawkeye, IllIllI, JMukesh, Meta0xNull, PPrieditis, Picodes, TerrierLover, Tomio, WatchPug, berndartmueller, catchup, delfin454000, dirk_y, ellahi, hickuphh3, ilan, kebabsec, kenta, nahnah, rayn, rfa, robee, rokinot, saian, securerodd, slywaters, sorrynotsorry
80.9074 USDC - $80.91
Loops should use the same pattern:
function LPFarming.claimAll() breaks the pattern at: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L348
At the same time pattern is used in function LPFarming._massUpdatePools() https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L280-L281
Use the same loop gas saving pattern consistently
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. Custom errors decrease both deploy and runtime gas costs. Source: https://blog.soliditylang.org/2021/04/21/custom-errors/
OpenZeppelin is also planing to use custom errors starting with next major release 5.0. Source release v5.0: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2961 Source OZ custom error issue: https://github.com/OpenZeppelin/openzeppelin-contracts/issues/2839
Refactor code and use Solidity custom errors.
claim() and claimAll() are very similar functions and aim of claimAll() is to save gas. If one function needs to be changed then most likely it is also necessary to change second function accordingly due to code duplication. It would be better to have only one function in place of two.
Remove function claimAll() and send a uint256 array of _pid to function claim() in place of a single number. Change code: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L332-L334 From: function claim(uint256 _pid) external nonReentrant noContract(msg.sender) { _updatePool(_pid); _withdrawReward(_pid); ... to: function claim(uint256[] _arrayPid) external nonReentrant noContract(msg.sender) { uint256 length = _arrayPid.length; for (uint256 i; i < length; ++i) { _updatePool(_arrayPid[i]); _withdrawReward(_arrayPid[i]); } ...
uint != 0 is more efficient than uint > 0
overrideFloor() should use "_newFloor != 0" in place of "_newFloor > 0" https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L278
Calldata is almost the same as memory but you cannot manipulate (change) the variable. It's also a bit more gas efficient. Use memory if you want to be able to manipulate the values and calldata when you don't. https://www.reddit.com/r/ethdev/comments/p6a00k/when_to_use_memory_storage_and_callldata/
Use calldata: https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L212 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L222 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L232 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L247 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L290 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L300 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L311 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L388-L389 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L400 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L484 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L606 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L632