JPEG'd contest - hubble's results

Summary of Findings for Low / Non-Critical issues

• L-01 : lower and upper bounds not checked during updating newFloor value
• L-02 : function setNFTTypeValueETH() should not allow setting value for bytes32(0) NFTType
• L-03 : validation check is very basic in function _validateRate()
• L-04 : missing events in some important functions in NFTVault.sol

Details L-01

Title : lower and upper bounds not checked during updating newFloor value

If the protocol wants to override the oracle with a custom floor value, it will use the function overrideFloor(uint256 _newFloor) However there is no input validation and the values can be set to any value. To mitigate any misuse or error, there should be some lower and upper bound values defined, say wrt to the oracle price.

Proof of Concept

Contract : NFTVault.sol
Function : overrideFloor(uint256 _newFloor)

Recommended Mitigation Steps

One solution is to take the oracle price as reference and check if the newFloor value is within some range, like

    oracle_price = _normalizeAggregatorAnswer(useFallbackOracle ? fallbackOracle : floorOracle);
    require(_newFloor > 0.5*oracle_price && _newFloor < 1.5*oracle_price);

Details L-02

Title : function setNFTTypeValueETH() should not allow setting value for bytes32(0) NFTType

The function setNFTTypeValueETH() can be used to set the value for any NFTType,

nftTypeValueETH[_type] = _amountETH;

Similarly the overrideFloor(uint256 _newFloor) is also used to set set the value

nftTypeValueETH[bytes32(0)] = _newFloor;

The cause of concern is that once a value is set for bytes32(0) using overrideFloor function, the value of bytes32(0) can be changed via setNFTTypeValueETH() function also, which should be prohibited.

Proof of Concept

Contract : NFTVault.sol Function : setNFTTypeValueETH(bytes32 _type, uint256 _amountETH)

Recommended Mitigation Steps

Add a check in setNFTTypeValueETH() for filtering out values of _type = bytes32(0)

require(_type != bytes32(0), "cannot update value for type = 0");

Details L-03

Title : validation check is very basic in function _validateRate()

_validationRate() does a very basic check for denominator >0 and denominator > numerator so that the variable value is not > 1. However there needs to be additional check so as to prevent rugpull, malicious activity or changes by mistake. Example, currently its possible to set the organizationFeeRate to 100%, the debtInterestApr to 100% or the insurancePurchaseRate to 100%

There should be additional check for the values to be within some max values. This will give some confidence to the users, and protection against some malicious events.

Proof of Concept

Contract : NFTVault.sol Function : _validateRate()

Recommended Mitigation Steps

Define MAX values for each of the rate, and check them additionally during change.

Details L-04

Title : missing events in some important functions in NFTVault.sol

There are some important functions, like below, line#232 : function setCreditLimitRate(Rate memory _creditLimitRate) line#247 : function setLiquidationLimitRate(Rate memory _liquidationLimitRate)

These should ideally generate events.

Recommended Mitigation Steps

Add relevant events during changing important values.