code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

The insuraceRepurchaseTimeLimit variable of the VaultSettings struct defines the buffer time period where a user can repurchase their NFT following a liquidation. However, this variable is missing a setter function and does not have its original value validated in the initialize() function.

In the worst case scenario, a value of 0 is passed for insuraceRepurchaseTimeLimit with no way to correct the mistake. A 0 second insuraceRepurchaseTimeLimit would effectively render the insurance functionality useless as the liquidator is able to liquidate and then immediately seize the NFT. The original owner has paid the insurance purchase fee in return for no utility.

Since this variable does not become important until a liquidation occurs, it is likely that this contract would have already seen adoption from users. The only way to update this variable to the desired value would be a migration to a new contract.

It's also a possibility that the variable was set with the desired value originally, but as time passes, the DAO decides on a more appropriate value. Again, the original value is permanent.

Proof of Concept

The DAO passes the VaultSettings into the initializer. Due to lack of validation, insuraceRepurchaseTimeLimit is set to 0.
User A deposits their NFT and borrows using insurance. Assuming an insurance purchase rate of 10%, the borrower receives 10% less stablecoin than the value of the NFT.
User defaults, therefore their position is liquidated.
Since the insuraceRepurchaseTimeLimit is 0, the liquidator can immediately seize the NFT collateral.

Therefore, User A has received 10% less PUSD due to paying for insurance, and the insurance did not provide them any benefit as their NFT was still seized.

Tools Used

Manual review.

Recommended Mitigation Steps

Add a setter function for this variable. Add validation to check that the value > 0 in the initialize function.

spaghettieth commented 2 years ago

From the contest's readme:

Wardens can assume that governance variables are set appropriately, and can reach out on Discord for any further clarification.

dmvt commented 2 years ago

Out of scope

code-423n4 / 2022-04-jpegd-findings