JPEG'd contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/e72861a9ccb707ced9015166fbded5c97c6991b6/contracts/lock/JPEGLock.sol#L56

Vulnerability details

Impact

If the owner incorrectly uses the lockFor method and calls it twice with the same _nftIndex, it will overwrite the record and the deposited jpeg can never be retrieved.

Proof of Concept

• Owner call lockFor(Alice,0,100);
• Owner call lockFor(Bob,0,100);
• 100 tokens are lost.

Recommended Mitigation Steps

It's mandatory to check that the position does not exist, or return the _nftIndex as a counter in this method.

1. It was found some transfer or transferFrom without checking the boolean result, ERC20 standard specify that the token can return false if this call was not made, so it's mandatory to check the result of approve methods.

• NFTVault.sol#L899
• JPEGStaking.sol#L34
• JPEGStaking.sol#L52

2. Lack of input checks.

• LPFarming.sol#L77

1. Change the incremental logic from i++ to ++i in order to save some opcodes:

• LPFarming.sol#L348
• NFTVault.sol#L181
• NFTVault.sol#L184
• StrategyPUSDConvex.sol#L145
• StrategyPUSDConvex.sol#L231
• StrategyPUSDConvex.sol#L319

2. Use delete instead of set to default value (false or 0)

• Controller.sol#L76
• NFTVault.sol#L380

3. It's compared a boolean value using == true or == false, instead of using the boolean value, or NOT opcode, it's cheaper to use NOT when the value it's false, or just the value without == true, when it's true, because it will use less opcode inside the VM.

• Controller.sol#L87

4. It's possible to optimize the following structs in order to save storage slots:

• NFTVault.sol#L610-L623

    struct PositionPreview {
        address owner;
        uint256 nftIndex;
        bytes32 nftType;
        uint256 nftValueUSD;
        VaultSettings vaultSettings;
        uint256 creditLimit;
        uint256 debtPrincipal;
        uint256 debtInterest;
        BorrowType borrowType; // move this two at the end, close to liquidator
        bool liquidatable;     // move this two at the end, close to liquidator
        uint256 liquidatedAt;
        address liquidator;
    }