QA Report

[code423n4]

Summary

We list 5 low-critical findings and 2 non-critical findings here:

• (Low) A address can be hijack to do bad thing
• (Low) setAvailableTokensRate() has a mistake requirement
• (Low) getNFTInfo does not check against invalid NFT
• (Low) Owner can extract all remaining rewards by specifying a new epoch
• (Low) The permission of STRATEGIST_ROLE is too powerful
• (Non) add function doesn’t return any information
• (Non) Uniswap v3 router does not have the quoteExactInput function

In conclusion, it's better to check invalid NFT, restrict owner’s permission and use minimum permission of roles.

(Low) A address can be hijack to do bad thing

Impact

In _executeTransfer of escrow/NFTEscrow.sol. Though it is extremely unlikely to happen, the non 0 chance of address collision with FlashEscrow may cause users to lose their NFT.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/escrow/NFTEscrow.sol#L68-L72

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Remove FlashEscrow, call _encodeFlashEscrowPayload() directly and use a mapping to record transactions relative to users.

(Low) setAvailableTokensRate() has a mistake requirement

Impact

The setAvailableTokensRate() function in vaults/yVault/yVault.sol should check denominator > 0, instead of numerator.

Proof of Concept

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

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

_rate.numerator > 0 should be _rate.denominator > 0:

        require(
            _rate.denominator > 0 && _rate.denominator >= _rate.numerator,
            "INVALID_RATE"
        );

(Low) getNFTInfo does not check against invalid NFT

Impact

getNFTInfo doesn’t check against invalid NFT, it will be mistaken for a valid NFT if other contracts call getNFTInfo.

Proof of Concept

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

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Use validNFTIndex to check validation.

(Low) Owner can extract all remaining rewards by specifying a new epoch

Impact

In farming/LPFarming.sol, Owner can call newEpoch() to extract all remaining rewards by specifying a new epoch.

Proof of Concept

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

Owner can create a new epoch with the same _endBlock and _startBlock, which will let newRewards equal to 0, and get all remainingRewards.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Check _endBlock - _startBlock and newRewards should be a valid value.

(Low) The permission of STRATEGIST_ROLE is too powerful

Impact

The STRATEGIST_ROLE can transfer any amount of token and call _strategy.withdraw without any check. It’s dangerous when a private key of a STRATEGIST_ROLE is stolen.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L131 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/yVault/Controller.sol#L141

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Remove inCaseTokensGetStuck and inCaseStrategyTokensGetStuck functions or restrict the ability of STRATEGIST_ROLE.

(Non) add function doesn’t return any information

Impact

add function in farming/LPFarming.sol doesn’t return any information (e.g. pid).

Proof of Concept

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

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

It’s better to return pid for other functions to use (e.g. set function).

(Non) Uniswap v3 router does not have the quoteExactInput function

Impact

In interface ISwapRouter of interfaces/ISwapRouter.sol, it define quoteExactInput but Uniswap V3 router doesn’t have this function.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/interfaces/ISwapRouter.sol#L23

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

Delete quoteExactInput function.