ParaSpace contest - Awesome's results

Lines of code

https://github.com/code-423n4/2022-11-paraspace/blob/main/paraspace-core/contracts/misc/NFTFloorOracle.sol#L326-L338

Vulnerability details

Impact

According to this comment, removeFeeder() is intended to be called only by the owner to remove the feeder.

However, because it is missing the modifier onlyRole() and since removeFeeder() is also external anyone can make a call to removeFeeder() and remove its feeder.

The owner may try to add feeders back by calling addFeeders(), but a malicious user can continuously call removeFeeder() to remove the feeder every time the owner adds the feeder back.

Depending on the price feed information and updates, this can disrupt the protocol on all function calls.

Proof of Concept

misc/NFTFloorOracle.sol#L165-L172

/// @notice Allows owner to remove feeder.
/// @param _feeder feeder to remove
function removeFeeder(address _feeder)
    external
    onlyWhenFeederExisted(_feeder)
{
    _removeFeeder(_feeder);
}

As we can see, removeFeeder() does not have an onlyRole() modifier to verify if the person calling is the owner.

Recommended Mitigation Steps

Add the modifier onlyRole() to the removeFeeder() function

1. Lines are too long

Usually, lines in source code are limited to 80 characters.

It's advised to keep lines lower than 120 characters.

Today’s screens are much larger so it’s reasonable to stretch this in some cases.

Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length

There is 1 instance of this issue

• LiquidationLogic.sol#L603

2. Typos

File: LiquidationLogic.sol

• LiquidationLogic.sol#L104

Line 104:   //userDebt from liquadationReserve

File: NFTFloorOracle.sol

• NFTFloorOracle.sol#L412, NFTFloorOracle.sol#L53, NFTFloorOracle.sol#L54

Line  53:    /// aggeregate prices which are not expired from different feeders, if number of valid/unexpired prices

Line  54:    /// not enough, we do not aggeregate and just use previous price

Line 412:    //aggeregate with price from all feeders

SignatureChecker.sol#L14

• SignatureChecker.sol#L14, SignatureChecker.sol#L15, SignatureChecker.sol#L43, SignatureChecker.sol#L48

Line 14:    * @param hash the hash containing the signed mesage

Line 15:    * @param v parameter (27 or 28). This prevents maleability since the public key recovery equation has two possible solutions.

Line 43:    * @param hash the hash containing the signed mesage

Line 48:    * @param domainSeparator paramer to prevent signature being executed in other chains and environments

Suggested changes:

• liquadationReserve => liquidationReserve
• aggeregate => aggregate
• mesage => message
• maleability => malleability
• paramer => parameter