JPEG'd contest - Kthere's results

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L59

Vulnerability details

Impact

The _lockAmount gets overwritten each time the lockFor function is called in JPEGLock.sol. If the function is called multiple times, the parameter will be updated wrongly.

Proof of Concept

The _lockAmount parameter gets assigned with the amount locked each time the lockFor function is called. If the function is called again for locking additional tokens for the same _account and _nftIndex. Then the amount will be overwritten instead of adding the amount to the current amount of tokens, thereby giving the wrong number of locked tokens in further calculations.

Tools Used

Manual Checks

Recommended Mitigation Steps

The following can be used instead:

lockAmount: lockAmount + _lockAmount

LPFarming.sol

• Line47: Incorrect natspec comment, replace starting with ending
• line76: Constructor can be fed with address(0), check required to avoid that
• line96: setContractWhitelisted function, requires a check to ensure the same value ( whitelisted contract) is not used for the call, and ensure address(0) checks are missing
• line107: No checks in place to ensure that any epoch is not currently happening
• line165: The allocation points can be set to zero, check required

yVaultLPFarming.sol

• line65: setContractWhitelisted function, requires a check to ensure the same value ( whitelisted contract) is not used for the call, and ensure address(0) checks are missing
• line134: claim function does not have a reentrancy guard

CryptoPunksHelper.sol

• Re-entrancy guard required for crypto punks transfer

JPEGLock.sol

• line49: lockFor function, no checks in place to avoid input amount=0

yVault.sol

• line39: Constructor needs address(0) checks
• line89: setContractWhitelisted function, requires a check to ensure the same value ( whitelisted contract) is not used for the call, and ensure address(0) checks are missing
• line166: Re-entrancy guard required for withdraw function

NFTVault.sol

• line203: setBorrowAmountCap, needs 0 value checks and checks required to avoid calling function for the existing values (input = current _borrowAmountCap)
• line212: setDebtInterestApr, needs checks to avoid calling function for the existing values (input = current _debtInterestApr)
• line222: setValueIncreaseLockRate, needs checks to avoid calling function for the existing values (input = current _valueIncreaseLockRate)
• line232: setCreditLimitRate, needs checks to avoid calling function for the existing values (input = current _creditLimitRate)
• line247: setLiquidationLimitRate, needs checks to avoid calling function for the existing values (input = current _liquidationLimitRate)
• line271: setJPEGLockTime,needs checks to avoid calling function for the existing values (input = current _lockTime)
• line277: overrideFloor, needs checks to avoid calling function for the existing values (input = current _floor)
• line290: setOrganizationFeeRate, needs checks to avoid calling function for the existing values (input = current _organizationFeeRate)
• line300: setInsurancePurchaseRate, needs checks to avoid calling function for the existing values (input = current _insurancePurchaseRate)
• line310: setInsuranceLiquidationPenaltyRate, needs checks to avoid calling function for the existing values (input = current _insuranceLiquidationPenaltyRate)
• line321: setNFTType, needs checks to avoid calling function for the existing values
• line336: setNFTTypeValueETH, needs checks to avoid calling function for the existing values
• line347: setPendingNFTValueETH, needs checks to avoid calling function for the existing values
• line610: Inefficient struct packing. uint256 variable occupies a complete slot (32 bytes), address takes 20 bytes and Boolean variables take 1 byte. Ensure the variables occupy minimum possible storage slots (eg. bool and address can be coupled together to occupy fewer slots as compared to the current packing in line620-line622). Also, check if other struct variables can be stored in smaller data types (uint96, uint128, uint64 - preferable for timestamps) in the rest of the codebase and pack them efficiently.