This limits the ability for off-chain monitoring tools to observe and react to changes to protocol parameters. It's considered a best practice to emit events for state changing operations, especially changes to key protocol parameters.
Avoid payable.transfer
ETH transfers in FungibleAssetVaultForDAO#withdraw are performed using payable(msg.sender).transfer.
It's considered a best practice to avoid this pattern for ETH transfers, since it forwards a fixed amount of gas and may revert if future gas costs change. (See the Consensys Diligence article here.)
Since the withdraw function is already nonReentrant, consider using OpenZeppelin's Address.sendValue instead.
Unbounded iteration over LP farming pools in _massUpdatePools
The _massUpdatePools function in LPFarming.sol iterates over all active and inactive LP pools. The contract owner may add, but not update or remove pools.
If the poolInfo array grows too large, add, set, and newEpoch may revert, impacting the ability to create and manage pools and preventing creation of new rewards epochs.
Additionally, the _updatePool function contains an external call to pool.lpToken.balanceOf. If a malicious or malfunctioning LP token is added to a configured pool, this call may revert.
Likelihood is mitigated since adding pools is a permissioned function and there will likely be a limited number of LP pools. Owner should monitor gas usage when adding a new pool, and take care to validate approved LP token addresses. Consider adding the ability to disable, remove, or reconfigure a pool.
function _updatePool(uint256 _pid) internal {
PoolInfo storage pool = poolInfo[_pid];
if (pool.allocPoint == 0) {
return;
}
uint256 blockNumber = _blockNumber();
//normalizing the pool's `lastRewardBlock` ensures that no rewards are distributed by staking outside of an epoch
uint256 lastRewardBlock = _normalizeBlockNumber(pool.lastRewardBlock);
if (blockNumber <= lastRewardBlock) {
return;
}
uint256 lpSupply = pool.lpToken.balanceOf(address(this));
if (lpSupply == 0) {
pool.lastRewardBlock = blockNumber;
return;
}
uint256 reward = ((blockNumber - lastRewardBlock) *
epoch.rewardPerBlock *
1e36 *
pool.allocPoint) / totalAllocPoint;
pool.accRewardPerShare = pool.accRewardPerShare + reward / lpSupply;
pool.lastRewardBlock = blockNumber;
}
Unbounded iteration over LP farming pools in claimAll()
The claimAll function in LPFarming.sol iterates over all active and inactive LP pools in the poolInfo array to claim rewards. The contract owner may add, but not remove pools.
If the poolInfo array grows too large, claimAll, may revert, preventing users from claiming accrued rewards. Additionally, the _updatePool function contains an external call to pool.lpToken.balanceOf that could revert if a malicious or malfunctioning LP token is added to pool configuration.
Severity is significantly mitigated since the user may still claim rewards from a single pool using claim(uint256). Likelihood is mitigated since adding pools is a permissioned function and there will likely be a limited number of LP pools. Owner should monitor gas usage when adding new pools.
function claimAll() external nonReentrant noContract(msg.sender) {
for (uint256 i = 0; i < poolInfo.length; i++) {
_updatePool(i);
_withdrawReward(i);
}
uint256 rewards = userRewards[msg.sender];
require(rewards > 0, "no_reward");
jpeg.safeTransfer(msg.sender, rewards);
userRewards[msg.sender] = 0;
emit ClaimAll(msg.sender, rewards);
}
QA/Informational
Duplicated noContract modifier
The noContract modifier, whitelistedContracts mapping, and setContractWhitelisted functions are duplicated across multiple contracts. Consider extracting this functionality to a shared abstract contract.
The Ownable() and ReentrancyGuard() calls included inline in the JPEGLock constructor may be omitted since these base contracts require no constructor arguments.
The local variable initializer inside the category initialization loop in NFTVault#constructor shadows the initializer modifier. Consider renaming this local variable.
Low
Missing events for parameter changes
Throughout the codebase, most state changing functions that update protocol parameters do not emit events.
For example:
NFTVault#setCreditLimitRate
NFTVault#setLiquidationLimitRate
NFTVault#toggleFallbackOracle
NFTVault#setBorrowAmountCap
NFTVault#setDebtInterestApr
NFTVault#setValueIncreaseLockRate
NFTVault#setJPEGLockTime
NFTVault#disableFloorOverride
NFTVault#setOrganizationFeeRate
NFTVault#setInsurancePurchaseRate
NFTVault#setInsuranceLiquidationPenaltyRate
This limits the ability for off-chain monitoring tools to observe and react to changes to protocol parameters. It's considered a best practice to emit events for state changing operations, especially changes to key protocol parameters.
Avoid payable.transfer
ETH transfers in
FungibleAssetVaultForDAO#withdraw
are performed usingpayable(msg.sender).transfer
.It's considered a best practice to avoid this pattern for ETH transfers, since it forwards a fixed amount of gas and may revert if future gas costs change. (See the Consensys Diligence article here.)
Since the
withdraw
function is alreadynonReentrant
, consider using OpenZeppelin'sAddress.sendValue
instead.Unbounded iteration over LP farming pools in
_massUpdatePools
The
_massUpdatePools
function inLPFarming.sol
iterates over all active and inactive LP pools. The contract owner may add, but not update or remove pools.If the
poolInfo
array grows too large,add
,set
, andnewEpoch
may revert, impacting the ability to create and manage pools and preventing creation of new rewards epochs.Additionally, the
_updatePool
function contains an external call topool.lpToken.balanceOf
. If a malicious or malfunctioning LP token is added to a configured pool, this call may revert.Likelihood is mitigated since adding pools is a permissioned function and there will likely be a limited number of LP pools. Owner should monitor gas usage when adding a new pool, and take care to validate approved LP token addresses. Consider adding the ability to disable, remove, or reconfigure a pool.
Code
LPFarming.sol#281
LPFraming.sol#300
Unbounded iteration over LP farming pools in
claimAll()
The
claimAll
function inLPFarming.sol
iterates over all active and inactive LP pools in thepoolInfo
array to claim rewards. The contract owner may add, but not remove pools.If the
poolInfo
array grows too large,claimAll
, may revert, preventing users from claiming accrued rewards. Additionally, the_updatePool
function contains an external call topool.lpToken.balanceOf
that could revert if a malicious or malfunctioning LP token is added to pool configuration.Severity is significantly mitigated since the user may still claim rewards from a single pool using
claim(uint256)
. Likelihood is mitigated since adding pools is a permissioned function and there will likely be a limited number of LP pools. Owner should monitor gas usage when adding new pools.Code
LPFarming.sol#348
QA/Informational
Duplicated
noContract
modifierThe
noContract
modifier,whitelistedContracts
mapping, andsetContractWhitelisted
functions are duplicated across multiple contracts. Consider extracting this functionality to a shared abstract contract.Usages:
yVaultLPFarming.sol#54
LPFarming.sol#85
yVault.sol#61
Constructor call without args can be omitted
The
Ownable()
andReentrancyGuard()
calls included inline in theJPEGLock
constructor may be omitted since these base contracts require no constructor arguments.JPEGLock#31
Suggested:
Local variable shadows initializer
The local variable
initializer
inside the category initialization loop inNFTVault#constructor
shadows theinitializer
modifier. Consider renaming this local variable.NFTVault.sol#182
Missing parameter documentation
Natspec documentation is missing for the
_jpeg
parameter inController#constructor
.Omit boolean check
Since
approvedStrategies[_token][_strategy]
returns a boolean value, you can safely omit the== true
check on line 87 ofController#setStrategy
.