Ondo Finance contest - Aymen0909's results

QA Report

Summary

Findings

1- getChainlinkOraclePrice function can return null prices :

The function getChainlinkOraclePrice is used in the OndoPriceOracleV2 contract to get a token price using the Chainlink oracle, and the function will revert if the price returned by the oracle is not greater or equal than zero, but if the returned price is zero (which can happen if there is an isssue in the Chainlink oracle for some reason) the function will not revert and still return the null price this can have a negative impact on other functions (from other contracts) that uses this price in their internal logic.

Risk : Low

Proof of Concept

Issue occurs in the getChainlinkOraclePrice function below :

File: lending/OndoPriceOracleV2.sol Line 277-301

function getChainlinkOraclePrice(
address fToken
) public view returns (uint256) {
    require(
        fTokenToOracleType[fToken] == OracleType.CHAINLINK,
        "fToken is not configured for Chainlink oracle"
    );
    ChainlinkOracleInfo memory chainlinkInfo = fTokenToChainlinkOracle[fToken];
    (
        uint80 roundId,
        int answer,
        ,
        uint updatedAt,
        uint80 answeredInRound
    ) = chainlinkInfo.oracle.latestRoundData();
    require(
        (answeredInRound >= roundId) &&
        (updatedAt >= block.timestamp - maxChainlinkOracleTimeDelay),
        "Chainlink oracle price is stale"
    );
    /**
        @audit : it should be answer > 0 and not answer >= 0
        This to avoid the null price in case of chainlink oracle issue
    */
    require(answer >= 0, "Price cannot be negative");
    // Scale to decimals needed in Comptroller (18 decimal underlying -> 18 decimals; 6 decimal underlying -> 30 decimals)
    // Scales by same conversion factor as in Compound Oracle
    return uint256(answer) * chainlinkInfo.scaleFactor;
}

As you can see the require() check allow a null price and so the function call will succeed and will return zero.

Mitigation

to avoid the issue of getting null prices the getChainlinkOraclePrice function shoud be modified to revert in that case, the follwing changes must be made :

function getChainlinkOraclePrice(
address fToken
) public view returns (uint256) {
    ...
    /**
        @audit : use answer > 0 
    */
    require(answer > 0, "Price cannot be negative");
    // Scale to decimals needed in Comptroller (18 decimal underlying -> 18 decimals; 6 decimal underlying -> 30 decimals)
    // Scales by same conversion factor as in Compound Oracle
    return uint256(answer) * chainlinkInfo.scaleFactor;
}

2- maxChainlinkOracleTimeDelay is too big for some tokens price feeds :

Risk : Low

The value of maxChainlinkOracleTimeDelay is used in the OndoPriceOracleV2 contract to check if the Chainlink oracle price is outdated or not and the current max delay value is set to 25h = 90000s :

File: lending/OndoPriceOracleV2.sol Line 77

uint256 public maxChainlinkOracleTimeDelay = 90000; // 25 hours

Even though this value may work for the majority of tokens certain tokens price feeds have a shorter heartbeat (heartbeat : the price feed update period) which may cause the Ondo oracle to give wrong (stale) prices for those tokens, for example BTC/USD or COMP/USD (there are others as well) have a heartbeat of 1h = 3600s so if those tokens prices are not updated for 25h the oracle will be providing very old prices which could cause problems for the protocol or any other party using those prices.

The different heartbeats of Chainlink oracle price feeds can be found here

Mitigation

To avoid this issue i would recommend to either set a lower value for maxChainlinkOracleTimeDelay variable (like 2h for example) or to use different delay values for some tokens (with lower heartbeats).

3- owner of JumpRateModelV2 contract can not be changed :

The JumpRateModelV2 contract does not use the OZ Ownable extension for handling ownership (compared to the OndoPriceOracle V1 & V2), instead the owner address state variable is declared and is set in the constructor and as the contract does not contain any functions to transfer ownership the owner can not be updated in the future which could cause problems.

Risk : Low

Proof of Concept

Issue occurs in the instance below :

File: lending/JumpRateModelV2.sol Line 66

owner = owner_;

Mitigation

Use the OZ Ownable extension for handling ownership of the JumpRateModelV2 contract.

4- Related data should be grouped in a struct :

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: lending/OndoPriceOracleV2.sol Line 55-73

55      mapping(address => OracleType) public fTokenToOracleType;
58      mapping(address => uint256) public fTokenToUnderlyingPrice;
62      mapping(address => address) public fTokenToCToken;
70      mapping(address => ChainlinkOracleInfo) public fTokenToChainlinkOracle;
73      mapping(address => uint256) public fTokenToUnderlyingPriceCap;

Mitigation

Group the related data in a struct and use one mapping :

struct fToken {
    OracleType fTokenToOracleType;
    uint256 fTokenToUnderlyingPrice;
    address fTokenToCToken;
    ChainlinkOracleInfo fTokenToChainlinkOracle;
    uint256 fTokenToUnderlyingPriceCap;
}

And it would be used as a state variable :

mapping(address => fToken) public _fTokens;

5- Named return variables not used anywhere in the function :

When Named return variable are declared they should be used inside the function instead of the return statement or if not they should be removed to avoid confusion.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: cash/CashManager.sol Line 784

returns (uint256 totalCashAmountRefunded)

Mitigation

Either use the named return variables inplace of the return statement or remove them.

6- Use solidity time-based units :

Solidity contains time-based units (seconds, minutes, hours, days and weeks) which should be used when declaring time based variables/constants instead of using literal numbers, this is done for better readability and to avoid mistakes.

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: lending/OndoPriceOracleV2.sol Line 77

uint256 public maxChainlinkOracleTimeDelay = 90000; // 25 hours

Mitigation

Use time units when declaring time-based variables.

Gas Optimizations

Summary

Findings

1- Multiple address/IDs mappings can be combined into a single mapping of an address/id to a struct :

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.

There are 5 instances of this issue :

File: lending/OndoPriceOracleV2.sol Line 55-73

55      mapping(address => OracleType) public fTokenToOracleType;
58      mapping(address => uint256) public fTokenToUnderlyingPrice;
62      mapping(address => address) public fTokenToCToken;
70      mapping(address => ChainlinkOracleInfo) public fTokenToChainlinkOracle;
73      mapping(address => uint256) public fTokenToUnderlyingPriceCap;

These mappings could be refactored into the following struct and mapping for example :

struct fToken {
    OracleType fTokenToOracleType;
    uint256 fTokenToUnderlyingPrice;
    address fTokenToCToken;
    ChainlinkOracleInfo fTokenToChainlinkOracle;
    uint256 fTokenToUnderlyingPriceCap;
}
    
mapping(address => fToken) public _fTokens;

2- State variables only set in the constructor should be declared immutable :

The state variables only set in the constructor should be declared immutable, this avoids a Gsset (20000 gas) in the constructor, and replaces each Gwarmacces (100 gas) with a PUSH32 (3 gas).

There is 1 instance of this issue:

File: lending/JumpRateModelV2.sol Line 24

address public owner;

3- storage variable should be cached into memory variables instead of re-reading them :

The instances below point to the second+ access of a state variable within a function, the caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read, thus saves 100gas for each instance.

There are 4 instances of this issue :

File: contracts/cash/CashManager.sol Line 221-229

mintRequestsPerEpoch[currentEpoch][msg.sender] += depositValueAfterFees;

emit MintRequested(
    msg.sender,
    currentEpoch,
    collateralAmountIn,
    depositValueAfterFees,
    feesInCollateral
);  

The value of currentEpoch is read twice from storage and its value doesn't change so in should be cached into memory, the code should be modified as follow :

uint256 _epoch = currentEpoch;
mintRequestsPerEpoch[_epoch][msg.sender] += depositValueAfterFees;

emit MintRequested(
    msg.sender,
    _epoch,
    collateralAmountIn,
    depositValueAfterFees,
    feesInCollateral
);    

File: contracts/cash/CashManager.sol Line 249-268

if (epochToExchangeRate[epochToClaim] == 0) {
    revert ExchangeRateNotSet();
}

...

emit MintCompleted(
    user,
    cashOwed,
    collateralDeposited,
    epochToExchangeRate[epochToClaim],
    epochToClaim
);   

The value of epochToExchangeRate[epochToClaim] is read twice from storage and its value doesn't change so in should be cached into memory, the code should be modified as follow :

uint256 _epochToExchangeRate = epochToExchangeRate[epochToClaim];
if (_epochToExchangeRate == 0) {
    revert ExchangeRateNotSet();
}

...

emit MintCompleted(
    user,
    cashOwed,
    collateralDeposited,
    _epochToExchangeRate,
    epochToClaim
);    

File: contracts/cash/CashManager.sol Line 296-318

if (exchangeRate > lastSetMintExchangeRate) {
    rateDifference = exchangeRate - lastSetMintExchangeRate;
} else if (exchangeRate < lastSetMintExchangeRate) {
    rateDifference = lastSetMintExchangeRate - exchangeRate;
}

uint256 maxDifferenceThisEpoch = (lastSetMintExchangeRate *
    exchangeRateDeltaLimit) / BPS_DENOMINATOR;

if (rateDifference > maxDifferenceThisEpoch) {
    epochToExchangeRate[epochToSet] = exchangeRate;
    _pause();
    emit MintExchangeRateCheckFailed(
    epochToSet,
    lastSetMintExchangeRate,
    exchangeRate
    );
} else {
    uint256 oldExchangeRate = lastSetMintExchangeRate;
    epochToExchangeRate[epochToSet] = exchangeRate;
    lastSetMintExchangeRate = exchangeRate;
    emit MintExchangeRateSet(epochToSet, oldExchangeRate, exchangeRate);
}    

In the code above the value of lastSetMintExchangeRate is read multiple times from storage and its value doesn't change till the last line so it should be cached into memory, the code should be modified as follow :

uint256 oldExchangeRate = lastSetMintExchangeRate;
if (exchangeRate > oldExchangeRate) {
    rateDifference = exchangeRate - oldExchangeRate;
} else if (exchangeRate < oldExchangeRate) {
    rateDifference = oldExchangeRate - exchangeRate;
}

uint256 maxDifferenceThisEpoch = (oldExchangeRate *
    exchangeRateDeltaLimit) / BPS_DENOMINATOR;

if (rateDifference > maxDifferenceThisEpoch) {
    epochToExchangeRate[epochToSet] = exchangeRate;
    _pause();
    emit MintExchangeRateCheckFailed(
    epochToSet,
    oldExchangeRate,
    exchangeRate
    );
} else {
    epochToExchangeRate[epochToSet] = exchangeRate;
    lastSetMintExchangeRate = exchangeRate;
    emit MintExchangeRateSet(epochToSet, oldExchangeRate, exchangeRate);
}      

File: contracts/cash/CashManager.sol Line 577-586

uint256 epochDifference = (block.timestamp - currentEpochStartTimestamp) /
    epochDuration;
if (epochDifference > 0) {
    currentRedeemAmount = 0;
    currentMintAmount = 0;
    currentEpoch += epochDifference;
    currentEpochStartTimestamp =
    block.timestamp -
    (block.timestamp % epochDuration);
}

The value of epochDuration is read twice from storage and its value doesn't change so in should be cached into memory, the code should be modified as follow :

uint256 duration = epochDuration;
uint256 epochDifference = (block.timestamp - currentEpochStartTimestamp) /
    duration;
if (epochDifference > 0) {
    currentRedeemAmount = 0;
    currentMintAmount = 0;
    currentEpoch += epochDifference;
    currentEpochStartTimestamp =
    block.timestamp -
    (block.timestamp % duration);
}  

4- Use unchecked blocks to save gas :

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn’t possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block.

There are 4 instances of this issue:

File: contracts/cash/CashManager.sol Line 210

uint256 depositValueAfterFees = collateralAmountIn - feesInCollateral; 

The above operation cannot underflow due to the check because the feesInCollateral is always less or equal to collateralAmountIn so the operation should be marked as unchecked.

File: contracts/cash/CashManager.sol Line 296-300

if (exchangeRate > lastSetMintExchangeRate) {
    rateDifference = exchangeRate - lastSetMintExchangeRate;
} else if (exchangeRate < lastSetMintExchangeRate) {
    rateDifference = lastSetMintExchangeRate - exchangeRate;
}

The two substractions in the code above cannot underflow because of the if check that precede them so they both should be marked as unchecked.

File: contracts/cash/CashManager.sol Line 626-630

if (collateralAmountIn > mintLimit - currentMintAmount) {
    revert MintExceedsRateLimit();
}

currentMintAmount += collateralAmountIn;

In the code above the value of currentMintAmount cannot overflow because the check before it ensures that we always have currentMintAmount less or equal to mintLimit even after the addition of collateralAmountIn, so the operation should be marked as unchecked.

File: contracts/cash/CashManager.sol Line 645-649

if (amount > redeemLimit - currentRedeemAmount) {
    revert RedeemExceedsRateLimit();
}

currentRedeemAmount += amount;

In the code above the value of currentRedeemAmount cannot overflow because the check before it ensures that we always have currentRedeemAmount less or equal to redeemLimit even after the addition of amount, so the operation should be marked as unchecked.

5- Unecessary initialization of currentEpoch in the constructor :

In the constructor of CashManager contract the value of currentEpoch is set to be equal to its current value which is just redundant and cost more gas, if this is an error then the correct value should be added otherwise this line of code should be removed to save gas at the deployment.

File: cash/CashManager.sol Line 168

currentEpoch = currentEpoch;

6- x += y/x -= y costs more gas than x = x + y/x = x - y for state variables :

Using the addition operator instead of plus-equals saves 113 gas as explained here

There are 8 instances of this issue:

File: contracts/cash/CashManager.sol

221     mintRequestsPerEpoch[currentEpoch][msg.sender] += depositValueAfterFees;
582     currentEpoch += epochDifference;
630     currentMintAmount += collateralAmountIn;
649     currentRedeemAmount += amount;
678     redemptionInfoPerEpoch[currentEpoch].addressToBurnAmt[msg.sender] += amountCashToRedeem;
681     redemptionInfoPerEpoch[currentEpoch].totalBurned += amountCashToRedeem;
864     redemptionInfoPerEpoch[epoch].totalBurned -= previousBalance - balance;
867     redemptionInfoPerEpoch[epoch].totalBurned += balance - previousBalance;

7- require() strings longer than 32 bytes cost extra gas :

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas.

There are 9 instances of this issue :

File: cash/factory/CashKYCSenderReceiverFactory.sol

163     require(
            msg.sender == guardian,
            "CashKYCSenderReceiverFactory: You are not the Guardian"
        );

File: cash/token/CashKYCSender.sol

63      require(
            _getKYCStatus(_msgSender()),
            "CashKYCSender: must be KYC'd to initiate transfer"
        );
70      require(
            _getKYCStatus(from),
            "CashKYCSender: `from` address must be KYC'd to send tokens"
        );

File: cash/token/CashKYCSenderReceiver.sol

63      require(
            _getKYCStatus(_msgSender()),
            "CashKYCSenderReceiver: must be KYC'd to initiate transfer"
        );
70      require(
            _getKYCStatus(from),
            "CashKYCSenderReceiver: `from` address must be KYC'd to send tokens"
        );
78      require(
            _getKYCStatus(from),
            "CashKYCSenderReceiver: `from` address must be KYC'd to send tokens"
        );

File: lending/OndoPriceOracle.sol

120     require(
            CTokenLike(fToken).underlying() == CTokenLike(cToken).underlying(),
            "cToken and fToken must have the same underlying asset if cToken nonzero"
        );

File: lending/OndoPriceOracleV2.sol

215     require(
            CTokenLike(fToken).underlying() == CTokenLike(cToken).underlying(),
            "cToken and fToken must have the same underlying asset"
        );
280     require(
            fTokenToOracleType[fToken] == OracleType.CHAINLINK,
            "fToken is not configured for Chainlink oracle"
        );

8- Splitting require() statements that uses && saves gas :

There is 1 instance of this issue :

File: lending/OndoPriceOracleV2.sol

192     require(
            (answeredInRound >= roundId) &&
                (updatedAt >= block.timestamp - maxChainlinkOracleTimeDelay),
            "Chainlink oracle price is stale"
        );

9- ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as in the case when used in for & while loops :

There are 9 instances of this issue:

File: contracts/cash/CashManager.sol

750     for (uint256 i = 0; i < size; ++i)
786     for (uint256 i = 0; i < size; ++i)
933     for (uint256 i = 0; i < size; ++i)
961     for (uint256 i = 0; i < exCallData.length; ++i)

File: contracts/cash/factory/CashFactory.sol

127     for (uint256 i = 0; i < exCallData.length; ++i) 

File: contracts/cash/factory/CashKYCSenderFactory.sol

137     for (uint256 i = 0; i < exCallData.length; ++i) 

File: contracts/cash/factory/CashKYCSenderReceiverFactory.sol

137     for (uint256 i = 0; i < exCallData.length; ++i) 

File: contracts/cash/kyc/KYCRegistry.sol

163     for (uint256 i = 0; i < length; i++) 
180     for (uint256 i = 0; i < length; i++)