QA Report

[code423n4]

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

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L203
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#L310

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

code-423n4/2022-01-livepeer-findings#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.