JPEG'd contest - catchup's results

Bridging the gap between DeFi and NFTs.

General Information

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

JPEG'd

Findings Distribution

Researcher Performance

Rank: 30/62

Findings: 2

Award: $252.10

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository JPEG'd

Findings Information

🌟 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

Awards

155.9803 USDC - $155.98

Labels

bug

QA (Quality Assurance)

sponsor disputed

External Links

Link to GitHub issue

C4 finding submitted: (risk = 1 (Low Risk)) noContract modifier may fail to verify if the account is an EOA

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L87 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L63 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L56

Vulnerability details

Impact

noContract modifier is intended to evaluate true either if the account address is a whitelisted contract or an EOA. However, isContract() would return false for the following types of addresses:

an externally-owned account
a contract in construction
an address where a contract will be created
an address where a contract lived, but was destroyed

Proof of Concept

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L11-L35

Tools Used

Manual analysis

Recommended Mitigation Steps

C4 finding submitted: (risk = 1 (Low Risk)) It would be better to integrate rounding error check within the function.

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L527-L539 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L637-L649

Vulnerability details

Impact

_calculateDebt() is said to be prone to rounding errors, so whenever it is called, an error check is applied which compares the return value to the debtPrincipal and picks the greater of two. It would be a better approach to include this check within the _calculateDebt() function, eliminating the danger of forgetting to include this check whenever _calculateDebt() is called.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Function can changed as below to include the rounding error check within the body:

function _calculateDebt(
    uint256 total,
    uint256 userPortion,
    uint256 totalPortion,
    uint256 _nftIndex
) internal view returns (uint256) {
	uint256 calculatedDebt = totalPortion == 0 ? 0 : (total * userPortion) / totalPortion;

    uint256 principal = positions[_nftIndex].debtPrincipal;

    //_calculateDebt is prone to rounding errors that may cause
    //the calculated debt amount to be 1 or 2 units less than
    //the debt principal when the accrue() function isn't called
    //in between the first borrow and the _calculateDebt call.
    return principal > calculatedDebt ? principal : calculatedDebt;
}

C4 finding submitted: (risk = 0 (Non-critical)) Floating pragma

Lines of code

All the contracts in scope, some of which https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L2 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L2 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/escrow/NFTEscrow.sol#L2

Vulnerability details

Impact

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Proof of Concept

https://swcregistry.io/docs/SWC-103

Tools Used

Manual analysis

Recommended Mitigation Steps

Lock the pragma

C4 finding submitted: (risk = 0 (Non-critical)) Important changes should emit events

Lines of code

Vulnerability details

Impact

Functions which are executed only by privileged users and change important parameters such as interest rates, credit caps, etc should emit events.

Proof of Concept

https://github.com/code-423n4/2022-01-livepeer-findings/issues/83

Tools Used

Manual analysis

Recommended Mitigation Steps

Add events for such changes.

C4 finding submitted: (risk = 0 (Non-critical)) Typo in comment

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L64

Vulnerability details

Impact

"Whether" is misspelled

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Fix the typo.

Findings Information

🌟 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

Awards

96.1225 USDC - $96.12

Labels

bug

G (Gas Optimization)

sponsor acknowledged

External Links

Link to GitHub issue

1- For loop index increments can be made unchecked. 2- Prefix increments are cheaper than postfix increments. 3- There is no need to set uint256 variable to 0 since default value is already 0.

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L281 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L348 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L181 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L184 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L145 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L231 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/strategies/StrategyPUSDConvex.sol#L319

Vulnerability details

Impact

There is no risk of overflowing the index of the for loops. Therefore, they can be changed to unchecked to save gas. This would save more gas as iterations in the loop increases. Also postfix increments can be changed as prefix as it is cheaper. Setting uint256 index to 0 is redundant since default value is already 0.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the index increments to unchecked and prefix such as: for (uint256 j ; j < initializer.nfts.length; ) { nftTypes[initializer.nfts[j]] = initializer.hash; unchecked { ++j; } }

Constant keccak variables can be changed to immutable.

Lines of code

Vulnerability details

Impact

When these variables defined as constant, keccak operation will be performed whenever the variable is used. However, if they are immutable, keccak hashing would be performed just once.

Proof of Concept

https://github.com/code-423n4/2021-10-slingshot-findings/issues/3

Tools Used

Manual analysis

Recommended Mitigation Steps

Change from constant to immutable.

Public variables can be private

Lines of code

Vulnerability details

Impact

Almost all of the public variables have public visibility where they can be private. Private state variables are cheaper.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change from public to private.

Error messages longer than 32 bytes

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L394 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L41 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L55 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/tokens/StableCoin.sol#L69

Vulnerability details

Impact

Error strings longer than 32 bytes are more expensive.

Proof of Concept

https://blog.polymath.network/solidity-tips-and-tricks-to-save-gas-and-reduce-bytecode-size-c44580b218e6#c17b

Tools Used

Manual analysis

Recommended Mitigation Steps

Limit the error strings to 32 bytes max.

Use custom errors for revert

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/helpers/CryptoPunksHelper.sol#L91 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/helpers/EtherRocksHelper.sol#L96 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/lock/JPEGLock.sol#L82 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L364 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/yVault.sol#L201 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/yVaultLPFarming.sol#L190

Vulnerability details

Impact

It is possible to use custom errors for revert statements starting from solidity 0.8.4. Custom errors are cheaper.

Proof of Concept

https://blog.soliditylang.org/2021/04/21/custom-errors/

Tools Used

Manual analysis

Recommended Mitigation Steps

Use custom errors for revert.

Use calldata for read-only arguments of external functions

Lines of code

Vulnerability details

Impact

When the read-only arguments of external functions are defined as memory, they are copied to memory space whereas they could have directly read from calldata.

Proof of Concept

https://github.com/code-423n4/2022-01-livepeer-findings/issues/61

Tools Used

Manual analysis

Recommended Mitigation Steps

Use calldata instead of memory where appropriate

Some internal functions can be made private

Lines of code

Vulnerability details

Impact

Calling private functions are cheaper than calling internal functions. Therefore, it is better to declare functions private if they are not called from inherited contracts.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Change the visibility to private where possible.

Strict inequalities are more expensive than non-strict inequalities

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L271 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L273

Vulnerability details

Impact

In the lines above non-strict inequlalities <= and >= can be used without disrupting the logic. Non-strict inequalities are cheaper.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

Use non-strict inequalities at these lines and where appropriate.

A couple of local variables can be deleted.

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L589-L595

Vulnerability details

Impact

All the calculation can be done in a single line by getting rid of the interestPerYear and interestPerSec local variables.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

The code can be changed as: // Accrue interest // uint256 interestPerYear = (totalDebtAmount * // settings.debtInterestApr.numerator) / // settings.debtInterestApr.denominator; // uint256 interestPerSec = interestPerYear / 365 days;

    return elapsedTime * totalDebtAmount * settings.debtInterestApr.numerator /
        settings.debtInterestApr.denominator / 365 days;

Caching can be avoided

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L503 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L517

Vulnerability details

Impact

_getNFTValueUSD(_nftIndex) can directly be used in the return statement, rather than being cached first.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

The code can be changed as: //uint256 asset_value = _getNFTValueUSD(_nftIndex); return (_getNFTValueUSD(_nftIndex) * settings.creditLimitRate.numerator) / settings.creditLimitRate.denominator;

organizationFee variable can be eliminated

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L713

Vulnerability details

Impact

organizationFee is calculated and then the value is assigned to feeAmount and after that point organizationFee is not used elsewhere. Thus, organizationFee can be replaced by feeAmount to save a variable and also an assignment.

Proof of Concept

Tools Used

Manual analysis

Recommended Mitigation Steps

The code can be changed as:

    //calculate the borrow fee
    uint256 feeAmount = (_amount *
        settings.organizationFeeRate.numerator) /
        settings.organizationFeeRate.denominator;

    //uint256 feeAmount = organizationFee;
    //if the position is insured, calculate the insurance fee

JPEG'd contest - catchup's results

Bridging the gap between DeFi and NFTs.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-36] QA Report - $155.98

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

[G-22] Gas Report - $96.12

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

Lines of code

Vulnerability details

Impact

Proof of Concept