JPEG'd contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the LPFarming contract, a new staking pool can be added using the add() function. The staking token for the new pool is defined using the _lpToken variable. However, there is no additional checking whether the _lpToken is the same as the reward token (jpeg) or not.

    function add(uint256 _allocPoint, IERC20 _lpToken) external onlyOwner {
        _massUpdatePools();

        uint256 lastRewardBlock = _blockNumber();
        totalAllocPoint = totalAllocPoint + _allocPoint;
        poolInfo.push(
            PoolInfo({
                lpToken: _lpToken,
                allocPoint: _allocPoint,
                lastRewardBlock: lastRewardBlock,
                accRewardPerShare: 0
            })
        );
    }

When the _lpToken is the same token as jpeg, reward calculation for that pool in the updatePool() function can be incorrect. This is because the current balance of the _lpToken in the contract is used in the calculation of the reward. Since the _lpToken is the same token as the reward, the reward minted to the contract will inflate the value of lpSupply, causing the reward of that pool to be less than what it should be.

    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;
    }

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L141-L154 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/farming/LPFarming.sol#L288-L311

Tools Used

None

Recommended Mitigation Steps

Add a check that _lpToken is not jpeg in the add function or mint the reward token to another contract to prevent the amount of the staked token from being mixed up with the reward token.

Lines of code

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

Vulnerability details

Impact

According to Chainlink's documentation, the latestAnswer function is deprecated. This function might suddenly stop working if Chainlink stop supporting deprecated APIs. And the old API can return stale data.

Proof of Concept

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L105 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L459

Tools Used

None

Recommended Mitigation Steps

Use the latestRoundData function to get the price instead. Add checks on the return data with proper revert messages if the price is stale or the round is uncomplete https://docs.chain.link/docs/price-feeds-api-reference/

Lines of code

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

Vulnerability details

Impact

Same as https://github.com/code-423n4/2022-01-openleverage-findings/issues/75

When a whitelisted user calls the withdraw function of the FungibleAssetVaultForDAO contract to withdraw native token, the whole user withdraw is being handled with a payable.transfer() call.

This is unsafe as transfer has hard coded gas budget and can fail when the user is a smart contract.

Whenever the user either fails to implement the payable fallback function or cumulative gas cost of the function sequence invoked on a native token transfer exceeds 2300 gas consumption limit the native tokens sent end up undelivered and the corresponding user funds return functionality will fail each time.

Proof of Concept

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

https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Tools Used

None

Recommended Mitigation Steps

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