JPEG'd contest - 0xDjango's results

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/yVault.sol#L142-L157

Vulnerability details

Impact

An early depositor to yVault.sol, preferably the first to deposit, will have the ability to steal funds from subsequent user deposits. The malicious user is able to do this by directly transferring tokens to either the yVault or Controller contracts following their deposit. If subsequent user deposits are less than the amount of token sent directly to the contracts, they will lose funds because their tokens will successfully transfer to the contract while they will be minted 0 shares

Proof of Concept

Steps to exploit:

• Malicious User A deposits 1 wei of token. User A receives 1 share since supply is currently 0 based on the following clause.

        if (supply == 0) {
            shares = _amount;

• Malicious User A then directly sends token to yVault.sol which will inflate the balance of the contract. For this example, User A sends 10,000 ether of token to the contract.
• The next user B attempts to deposit 5,000 ether of token. Based on the following formula, they will receive 0 shares even though the funds have already been transferred to the contract.

        } else {
            shares = (_amount * supply) / balanceBefore;
        }

Where balanceBefore is equal to balance() which is equal to token.balanceOf(address(this)) + controller.balanceOf(address(token)) The formula for the example looks like:

shares = (5,000 ether * 1 share) / 10,000 ether = 0 shares

• Malicious User A then calls withdraw(). Since the contracts hold 15,000 ether of tokens and User A holds the only share, they receive all 15,000 ether.

Tools Used

Manual review.

Recommended Mitigation Steps

Instead of relying on the balance of tokens of the two contracts, use internal accounting variables to represent the current balance of tokens that have been properly deposited and withdrawn from the vault. This will remove the ability to directly transfer tokens to the contracts to alter the share distribution logic.

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/FungibleAssetVaultForDAO.sol#L105 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L459

Vulnerability details

Impact

In the various calls to the Chainlink oracle, the deprecated API function latestAnswer() is used. This approach is vulnerable to price manipulation and stale prices according to the Chainlink documentation.

This vulnerability was marked as Medium severity in the following Halborn and Quantstamp audits.

Halborn (PAGE 19):

https://3405344147-files.gitbook.io/~/files/v0/b/gitbook-x-prod.appspot.com/o/spaces%2F6bWsvjSvuHlmjaYdDGxA%2Fuploads%2FxvaQXQq7NxRcRQBiGL3J%2FRolla_Finance_Quant_Protocol_Smart_Contract_Security_Audit_Report.pdf?alt=media&token=1d59da93-2e5c-4e53-9de8-a4bb6dba138e

Quantstamp (PAGE 4)

Download link on page. Page 4 of report explains vulnerability. https://docs.clipper.exchange/how-clipper-works/audits

Tools Used

Manual review.

Recommended Mitigation Steps

Per the Halborn audit:

Use the latestRoundData() function to get the price instead. Add checks on the return data with proper revert messages if the price is stale or the round is uncomplete, for example:

(
   roundId ,
   rawPrice ,
   ,
   updateTime ,
   answeredInRound
) = AggregatorV3Interface ( XXXXX ) . latestRoundData () ;
require ( rawPrice > 0 , " Chainlink price <= 0") ;
require ( updateTime != 0 , " Incomplete round ");
require ( answeredInRound >= roundId , " Stale price ");

Issue #1 (Low) - Floating Pragma

All contracts contain a floating pragma. It is recommended to deploy all contracts with a single, specific compiler version to reduce the risk of compiler-specific bugs and contracts deployed with different versions.

A single example can be found here: https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/tokens/JPEG.sol#L2

Issue #2 (Low) - Lack of CEI Pattern

In LPFarming.sol, the claim() and claimAll() functions do not follow a proper checks-effects-interactions pattern. There is no vulnerability in these functions due to the nonReentrant modifier and apparent lack of control flow transfer with the JPEG token, however, it is a simple security upgrade to move the userRewards[msg.sender] = 0 line above jpeg.safeTransfer(msg.sender, rewards);

Link to code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L339-L340

Issue #3 (Low) - Comment vs code conflict for newEpoch

As discussed in my previously submitted Medium finding called "Owner of LPFarming.sol can DOS rewards by stopping epoch at any time", there are two discrepancies between the comments of the newEpoch() function and the code implementation.

1. Comment says "@notice Allows the owner to start a new epoch. Can only be called when there's no ongoing epoch". There are no checks in the code whether there is an ongoing epoch or not.
2. Comment says "@param _startBlock The new epoch's start block. Has to be greater than the previous epoch's endBlock". There are no checks that the new epoch's start block is greater than the original epoch's end block.

Link to code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/LPFarming.sol#L103-L104

Issue #4 (Low) - Front-runnable initializer

Lack of access control on initialize(), can be front-run requiring re-deployment.

Link to code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L139-L149

Issue #5 (Low) - Input addresses should verify != 0

Several addresses set in the following initialize function do not verify that the input address are not the zero address.

Links to code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L170-L176

Issue #6 (Low) - Call to .decimals() which may not be supported for all tokens

Depending on the function used to deposit in yVault.sol, a call to token.decimals() may return errors since decimals() is not and ERC20 standard function.

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/yVault/yVault.sol#L70-L72

Issue #7 (Low) - Comment vs Code conflict for setContractWhitelisted

The setContractWhitelisted() function explains that calling this function allows the owner to whitelist contracts. This is slightly untrue since the function does not include any logic to determine if the address is a contract or not. Therefore, EOA's can be added to the contract address. The logic to decipher if the account is a contract or EOA is used higher up in the same contract. Adding a check for only contracts will make the comments match the function execution.

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/farming/yVaultLPFarming.sol#L65-L70

Issue #8 (Non-critical) - Insurance typo

Insurace should be spelled insurance.

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

Issue #9 (Non-critical) - Condition order continuity

It would make sense to order the conditions of the following statements in the same fashion to increase code clarity.

address(0) == positionOwner[_nftIndex]

positionOwner[_nftIndex] == address(0)

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L682-L695

Gas Optimization #1

Multiple require statements consume less gas than &&. This example is true for most setter functions for rates in this contract.

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L401-L404

Gas Optimization #2

This struct could benefit slightly from struct packing, e.g. address next to bool

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/vaults/NFTVault.sol#L610-L623