JPEG'd contest - kenta's results

2022-04-jpegd

1 follow the 𝐂𝐇𝐄𝐂𝐊 𝐄𝐅𝐅𝐄𝐂𝐓𝐒 𝐈𝐍𝐓𝐄𝐑𝐀𝐂𝐓𝐈𝐎𝐍 𝐏𝐚𝐭𝐭𝐞𝐫𝐧 in deposit and claimAll. The function deposit and claimAll don’t follow the check effects interaction pattern.

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

Update user.amount before transferFrom and userRewards[msg.sender] in claimAll.

2 use safeTransfer and safeTransferFrom in JPEGStaking.sol. SafeERC20Upgradeable is imported for IERC20Upgradeable. However, it is not used. You must use it in stake and unstake.

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/staking/JPEGStaking.sol#L34 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/staking/JPEGStaking.sol#L52

jpeg.safeTransferFrom(msg.sender, address(this), _amount); jpeg.safeTransfer(msg.sender, _amount);

3 use call to transfer ETH. the following line uses transfer to send ETH. However, call is recommended to send ETH. You need also check the return value of call if the transfer is successful is or not.

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

if (collateralAsset == ETH) { (bool success, ) = msg.sender.call{value: amount}(""); require(success, “Transfer failed”); }

4 import safeERC20 and use safeTransferFrom instead of transferFrom in repurchase. In the following line, transferFrom is used. You can replace it with safeTransferFrom to transfer ERC20 token.

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

using safeERC20 for IStableCoin; stablecoin.safeTransferFrom(msg.sender, position.liquidator, debtAmount + penalty);

5 Lock pragmas to specific compiler version. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

For example,

pragma solidity 0.8.0;

2022-04-jpegd gas optimization

1 delete cache for pool in deposit. The cached pool is used only one time in deposit, so you can delete it and save a little bit of gas cost.

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

deposit() gas will be from Avg 112668 to 112664 with this change according to the hardhat gas reporter.

2 Use unchecked for user.amount -= _amount. The underflow for this calculation is already checked in require statement, so you can use unchecked to save gas.

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

unchecked { user.amount -= _amount; }

withdraw() gas will be from Avg 85044 to 84891 with this change according to the hardhat gas reporter.

3 delete cache for pool in withdraw. The cached pool is used only one time in withdraw, so you can delete it and save a little bit of gas cost.

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

withdraw() gas will be from Avg 85044 to 85042 with this change according to the hardhat gas reporter.

4 use initial value for i und unchecked for increment in the loop.

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

For example,

for (uint256 pid; pid < length;) { // something executed unchecked { ++pid; } }

For example set() gas will be from Avg 41715 to 41611 with this change according to the hardhat gas reporter.

5 use unchecked for collateralAmount -= amount in FungibleAssetVaultForDAO. Underflow will never happen because require statement checked if collateralAmount is greater or equal than amount.

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

unchecked { collateralAmount -= amount; }

6 code duplication. The functions closePosition, liquidate, repurchase and claimExpiredInsuranceNFT have code duplication to delete position. Create an new internal function to save gas cost for deployment.

positionOwner[_nftIndex] = address(0); delete positions[_nftIndex]; positionIndexes.remove(_nftIndex);

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L808-L810 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L865-L867 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L906-L908 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/NFTVault.sol#L934-L936

For example, function deletePosition(uint256 _nftIndex) internal { positionOwner[_nftIndex] = address(0); delete positions[_nftIndex]; positionIndexes.remove(_nftIndex);
}