code-423n4 / 2022-04-jpegd-findings

1 stars 1 forks source link

LPFarming: large `allocPoints` can cause overflow and lock funds #146

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

Setting the LPFarming pool allocPoint to a large value may cause _updatePool to overflow and if that were to happen then funds are unrecoverable.

Proof of Concept

Add this to the LPFarming.ts test suite:

    let blockNumber = await ethers.provider.getBlockNumber();
    await farming.newEpoch(blockNumber + 1, blockNumber + 100000000000000, 100);
    await jpeg.transfer(contract.address, await jpeg.balanceOf(owner.address));

    // Add a pool with a very large allocation
    await farming.add(ethers.constants.MaxInt256.div(100000), lpTokens[0].address);

    await lpTokens[0].transfer(alice.address, units(1000));
    await lpTokens[0].transfer(bob.address, units(1000));

    await lpTokens[0].approve(farming.address, units(10000));
    await lpTokens[0].connect(alice).approve(farming.address, units(1000));
    await lpTokens[0].connect(bob).approve(farming.address, units(1000));

    // Pool accepts deposits
    await farming.deposit(0, units(100));

    await mineBlocks(1000);

    // However claim reverts!
    await farming.claim(0);

    // Attempting to withdraw will revert as well
    await farming.withdraw(0, units(100));

Tools Used

Added a test to the current test suite.

Recommended Mitigation Steps

Cap allocPoints in add and set to a reasonable max value so that pendingReward and _updatePool cannot overflow.

spaghettieth commented 2 years ago

Duplicate of #58

dmvt commented 2 years ago

As described in #58, even if this did occur, it could be immediately corrected. Invalid.