search

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

1 stars 1 forks source link

[WP-H20] A malicious early user/attacker can manipulate the `accRewardPerShare` to break `deposit` #166

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

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

function _computeUpdate() internal view returns (uint256 newAccRewardsPerShare, uint256 currentBalance) {
    currentBalance = vault.balanceOfJPEG() + jpeg.balanceOf(address(this));
    uint256 newRewards = currentBalance - previousBalance;

    newAccRewardsPerShare = accRewardPerShare + newRewards * 1e36 / totalStaked;
}

accRewardPerShare will monotonically increase. When the number becomes too large, most essential features of the contract can be broken due to overflow.

A malicious early user/attacker can manipulate it and break the contract's essential features, and even freeze users' funds.

PoC

  1. Alice deposited 1 wei, totalStaked = 1
  2. Alice sent 10 * 1e18 jpeg to yVaultLPFarming and called claim():
  1. Bob calls deposit 100,000 (1e23 wei), the transaction will fail due to overflow in _withdrawReward():

Recommendation

Consider requiring a minimum stake amount for the first depositor, that should make the manipulation much harder.

spaghettieth commented 2 years ago

No funds are at risk, this issue would just prevent users from depositing, which would easily be solved by migrating farm.

dmvt commented 2 years ago

Sponsor is correct. There are no funds at risk with this exploit. At worst this is a gas issue for the user, but since warden did not mention gas it can't really be considered a gas report. Invalid.