JPEG'd contest - minhquanym's results

Lines of code

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

Vulnerability details

Impact

In both function _updatePool and pendingReward, lpSupply of that pool is calculated as pool.lpToken.balanceOf(address(this)).

But balance of lpToken can increase not only by calling deposit in that pool but also by deposit in another pool which has the same lpToken or simply by transfering lpToken directly to LPFarming contract.

In that case, accRewardPerShare is calculated using wrong amount of lpToken supply and its value will be lower than expected and total reward all users able to claim will decrease.

Proof-of-concept

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

Consider the following scenario

• LPFarming currently has 2 pools A and B with the same lpToken X. Both have the same allocPoint = 10. Total reward is 5000 JPEG. So each pool is allocated 2500 JPEG reward.
• Alice deposit 500 token X into A, now lpSupply of both A and B is 500.
• Bob deposit 500 token X into B, now lpSupply of both A and B is 1000.
• Someone by accident or malicious user transfers 5000 token X to address of LPFarming, now lpSupply of both A and B is 6000.
• When the epoch end, Alice claims her reward. Since she is the only user in pool A, she supposed to claim all 2500 JPEG reward. But actually she only received 2500 * 500 / 6000 = 208.33 JPEG.
• The situation is the same for Bob.

Tools Used

Manual code review

Recommended Mitigation Steps

Add 1 more variable into PoolInfo struct to keep track of lpSupply.

struct PoolInfo {
  IERC20 lpToken;
  uint256 allocPoint;
  uint256 lastRewardBlock;
  uint256 accRewardPerShare;
  uint256 lpSupply;
}

And change line 190 and 300 to

uint256 lpSupply = pool.lpSupply;

1. Multiply before divide in calculation for better accuracy

Impact

In function NFTVault._calculateAdditionalInterest we should multiply by elapsedTime first then divide by 365 days for better accuracy.

We are working with uint so the result of operator ‘/’ is the quotient.

For example: (5 / 2) * 4 = 2 * 4 = 8 (5 * 4) / 2 = 20 / 2 = 10

Proof-of-concept

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

Tools Used

Manual code review

Recommended Mitigation Steps

Change line 593 - 595 to

return (interestPerYear * elapsedTime) / 365 days;

Lines of code

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

Vulnerability details

Impact

Function FungibleAssetVaultForDAO.withdraw in line 201 uses native transfer function to send ETH to msg.sender.

This is unsafe as transfer has hard coded gas budget (2300 gas) and can fail when the user is a smart contract. Especially when this contract is for DAO and ecosystem contracts as documentation.

Proof-of-concept

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

Tools Used

Manual code review

Recommended Mitigation Steps

All functions have a nonReentrant modifier already, so reentrancy is not an issue and transfer() can be replaced.

Using low-level call.value(amount) with the corresponding result check or using the OpenZeppelin Address.sendValue is advised https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L60