Inverse Finance contest - Aymen0909's results

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L82 https://github.com/code-423n4/2022-10-inverse/blob/main/src/Oracle.sol#L116

Vulnerability details

Impact

The Oracle contract is used to get the latest price for the users collateral tokens, but the functions getPrice and viewPrice use a deprecated Chainlink function latestAnswer to get the price of a given token as it's mentionned here, this function does not throw an error if no answer has been reached but returns 0 and it can give a stale price for a given token which would impact the user borrowing power.

Proof of Concept

There are 2 instances where the function latestAnswer is used :

File: src/Oracle.sol Line 82

uint price = feeds[token].feed.latestAnswer();

File: src/Oracle.sol Line 116

uint price = feeds[token].feed.latestAnswer();

Even though the issue concerning the function latestAnswer not reverting if no answer has been reached but returning 0 is handled in both getPrice and viewPrice functions with the check :

require(price > 0, "Invalid feed price");

The issue of stale prices still remains which could give a wrong borrowing power for the users, for example if for some reason the returned price is old by 2 days it could be the same as the two days low price and thus it will give wrong borrowing power to the user (higher or lower), not to mention that this stale price will also be stored and used for futher calculations in the future.

Tools Used

Manual review

Recommended Mitigation Steps

To avoid the risk of getting stale price from chainlink oracle i recommend to use the latestRoundData function with it's required checks.

The functions getPrice and viewPrice should be modified as follow :

function getPrice(address token, uint collateralFactorBps) external returns (uint) {
    ...
    
    // get price from feed
    (uint80 roundID ,uint price,, uint256 timestamp, uint80 answeredInRound) = feeds[token].feed.latestRoundData();
    require(price > 0, "Invalid feed price");
    require(answeredInRound >= roundID, "Stale price");
    require(timestamp != 0, "Round not complete");
    
    ...
}

function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
    ...
    
    // get price from feed
    (uint80 roundID ,uint price,, uint256 timestamp, uint80 answeredInRound) = feeds[token].feed.latestRoundData();
    require(price > 0, "Invalid feed price");
    require(answeredInRound >= roundID, "Stale price");
    require(timestamp != 0, "Round not complete");
    
    ...
}

QA Report

Summary

Findings

1- changeSupplyCeiling should check that _supplyCeiling >= globalSupply when setting a new ceiling :

The changeSupplyCeiling function inside Fed contract is called by the governance to set a new ceiling, but the function doesn't check that the new ceiling is lower than the current globalSupply, so if a new supplyCeiling greater than the current global supply is set this can temporarily block the expansion function and thus the supply to the borrow market.

Risk : Low

Proof of Concept

The changeSupplyCeiling function below doesn't check the new value for the supply ceiling (called _supplyCeiling) :

function changeSupplyCeiling(uint256 _supplyCeiling) public {
    require(msg.sender == gov, "ONLY GOV");
    supplyCeiling = _supplyCeiling;
}

So if the new set supplyCeiling is lower than the current globalSupply this will block the calls to the expansion function because of the following check :

require(globalSupply <= supplyCeiling);

The consequence of this is that DOLA tokens cannot be minted to the borrow markets until a new voting occurs to set a new supplyCeiling (which could take some time depending on the voting period), or the chair will be forced to call contraction function to decrease the current globalSupply but this can only work if the borrow market has sufficient DOLA balance.

Mitigation

Add a check inside the changeSupplyCeiling function to avoid this issue, the function should be modified as follow :

function changeSupplyCeiling(uint256 _supplyCeiling) public {
    require(msg.sender == gov, "ONLY GOV");
    require(globalSupply <= _supplyCeiling, "Invalid supply ceiling");
    supplyCeiling = _supplyCeiling;
}

2- change of operator in BorrowController should use two-step transfer pattern :

The change of operator in the BorrowController contract is done through a simple setter function but in the Oracle contract this is done with a 2-step transfer, the operator in both cases has a big role in setting/changing critical protocol parameters. So i recommend to also use the 2-step transfer pattern in the BorrowController contract to have more security when changing the operator.

Risk : Low

Proof of Concept

Instances include:

File: src/BorrowController.sol

function setOperator(address _operator)

Mitigation

Consider using 2-step transfer pattern when setting a new operator in the BorrowController contract.

3- Immutable state variables lack zero value (address or uint) checks :

Constructors should check that the values written in an immutable state variables variable are not zero (for uint) or the zero address (for addresses).

Risk : Low

Proof of Concept

Instances include:

File: src/Fed.sol

dbr = _dbr;

dola = _dola;

File: src/Market.sol

escrowImplementation = _escrowImplementation;

dbr = _dbr;

collateral = _collateral;

Mitigation

Add non-zero address checks in the constructors for the instances aforementioned.

4- require()/revert() statements should have descriptive reason strings :

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: src/Fed.sol Line 93

require(globalSupply <= supplyCeiling);

Mitigation

Add reason strings to the aforementioned require statements for better comprehension.

5- Event should be emitted in setters :

Setters should emit an event so that Dapps and users can detect important changes to critical protocol parameters like : fees, collateral factor...

Risk : NON CRITICAL

Proof of Concept

Instances include:

File: src/DBR.sol

function setReplenishmentPriceBps(uint newReplenishmentPriceBps_)

File: src/Market.sol

function setCollateralFactorBps(uint _collateralFactorBps)

function setLiquidationFactorBps(uint _liquidationFactorBps)

function setReplenismentIncentiveBps(uint _replenishmentIncentiveBps)

function setLiquidationIncentiveBps(uint _liquidationIncentiveBps)

function setLiquidationFeeBps(uint _liquidationFeeBps)

File: src/Oracle.sol

function setFeed(address token, IChainlinkFeed feed, uint8 tokenDecimals)

function setFixedPrice(address token, uint price)

Mitigation

Emit an event in all the aforementioned setters.

6- Duplicated require() checks should be refactored to a modifier :

require() checks statement used multiple times inside a contract should be refactored to a modifier for better readability.

Risk : NON CRITICAL

Proof of Concept

There are 3 instances of this issue in Fed.sol:

File: src/Fed.sol

49      require(msg.sender == gov, "ONLY GOV");
58      require(msg.sender == gov, "ONLY GOV");
67      require(msg.sender == gov, "ONLY GOV");

Those checks should be replaced by a onlyGov modifier as follow :

modifier onlyGov(){
    require(msg.sender == gov, "ONLY GOV");
    _;
}

There are another 3 instances of this issue in Fed.sol:

File: src/Fed.sol

76      require(msg.sender == chair, "ONLY CHAIR");
87      require(msg.sender == chair, "ONLY CHAIR");
104     require(msg.sender == chair, "ONLY CHAIR");

Those checks should be replaced by a onlyChair modifier as follow :

modifier onlyChair(){
    require(msg.sender == chair, "ONLY CHAIR");
    _;
}

7- public functions not called by the contrat should be declared external instead :

Impact - NON CRITICAL

Proof of Concept

There are many instances across the contracts :

File: src/BorrowController.sol

function setOperator(address _operator) public

function allow(address allowedContract) public

function deny(address deniedContract) public

function borrowAllowed(address msgSender, address, uint) public

File: src/DBR.sol

function setPendingOperator(address newOperator_) public

function setReplenishmentPriceBps(uint newReplenishmentPriceBps_) public

function claimOperator() public

function addMinter(address minter_) public

function removeMinter(address minter_) public

function addMarket(address market_) public

function totalSupply() public

function signedBalanceOf(address user) public

function approve(address spender, uint256 amount) public

function onBorrow(address user, uint additionalDebt) public

function onForceReplenish(address user, uint amount) public

File: src/Fed.sol

function changeGov(address _gov) public

function changeSupplyCeiling(uint _supplyCeiling) public

function changeChair(address _chair) public

function resign() public

function expansion(IMarket market, uint amount) public

function contraction(IMarket market, uint amount) public

function takeProfit(IMarket market) public

File: src/Market.sol

function setOracle(IOracle _oracle) public

function setBorrowController(IBorrowController _borrowController)

function setGov(address _gov) public

function setLender(address _lender) public

function setPauseGuardian(address _pauseGuardian) public

function setCollateralFactorBps(uint _collateralFactorBps) public

function setLiquidationFactorBps(uint _liquidationFactorBps) public

function setReplenismentIncentiveBps(uint _replenishmentIncentiveBps) public

function setLiquidationIncentiveBps(uint _liquidationIncentiveBps) public

function setLiquidationFeeBps(uint _liquidationFeeBps) public

function recall(uint amount) public

function pauseBorrows(bool _value) public

function forceReplenish(address user, uint amount) public

function liquidate(address user, uint repaidDebt) public

File: src/Market.sol

function setPendingOperator(address newOperator_) public

function setFeed(address token, IChainlinkFeed feed, uint8 tokenDecimals) public

function setFixedPrice(address token, uint price) public

function claimOperator() public

Mitigation

Declare the functions aforementioned external if they are not intended to be called inside the contract in the future.

Gas Optimizations

Summary

Findings

1- Remove redundant checks in onForceReplenish function (saves 1120 gas) :

The function onForceReplenish from the DBR.sol contract can only be called from the market using the forceReplenish function which already contains the checks around the user deficit, so it's redundant to verify the same checks in the onForceReplenish function and it conts more gas.

As you can see in the code below the first lines of the forceReplenish function :

File: src/Market.sol Line 560-562

function forceReplenish(address user, uint256 amount) public {
        uint256 deficit = dbr.deficitOf(user);
        require(deficit > 0, "No DBR deficit");
        require(deficit >= amount, "Amount > deficit");

        ...

}

And here are the first line of the onForceReplenish function :

File: src/DBR.sol Line 327-329

function onForceReplenish(address user, uint256 amount) public {
        require(markets[msg.sender], "Only markets can call onForceReplenish");
        uint256 deficit = deficitOf(user);
        require(deficit > 0, "No deficit");
        require(deficit >= amount, "Amount > deficit");

        ...

}

The first check in the onForceReplenish function ensure that only the forceReplenish function can call this function, and all other checks are the same in both functions.

The onForceReplenish function should be modified as follow :

function onForceReplenish(address user, uint256 amount) public {
    require(markets[msg.sender], "Only markets can call onForceReplenish");
    // @audit removed the redundant checks
    uint256 replenishmentCost = (amount * replenishmentPriceBps) / 10000;
    accrueDueTokens(user);
    debts[user] += replenishmentCost;
    _mint(user, amount);
}

This will save 1120 gas on average according to the tests.

2- Multiple address mappings can be combined into a single mapping of an address 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 6 instances of this issue :

The first one :

File: src/DBR.sol

26       mapping (address => uint) public debts;
27       mapping (address => uint) public dueTokensAccrued;
28       mapping (address => uint) public lastUpdated; 

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

struct BorrowInfo {
    uint256 debt;
    uint256 dueTokensAccrued;
    uint256 lastUpdated;
}
    
mapping(address => BorrowInfo) private _usersBorrow;

And the second is :

File: src/Market.sol

57       mapping (address => IEscrow) public escrows; 
58       mapping (address => uint) public debts; 
59       mapping(address => uint256) public nonces; 

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

struct UserInfo {
    IEscrow escrow;
    uint256 debt;
    uint256 nonce;
}
    
mapping(address => UserInfo) private _usersInfo;

3- 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.

Consider wrapping with an unchecked block here (around 25 gas saved per instance), there are 3 instances of this issue:

File: src/Market.sol Line 534-535

debts[user] -= amount;
totalDebt -= amount;

The above operations cannot underflow due to the check :

require(debt >= amount, "Insufficient debt");

And because totalDebt only get increase/decreased when debts[user] changes, the totalDebt can underflow if debts[user] doesn't underflow

File: src/Market.sol Line 599-600

debts[user] -= repaidDebt;
totalDebt -= repaidDebt;

The above operations cannot underflow due to the check :

require(repaidDebt <= debt * liquidationFactorBps / 10000, "Exceeded liquidation factor");

The liquidationFactorBps is always lower than 10000 so repaidDebt is always lower than debts[user] and because totalDebt only get increase/decreased when debts[user] changes, the totalDebt can underflow if debts[user] doesn't underflow.

File: src/Fed.sol Line 110-110

supplies[market] -= amount;
globalSupply -= amount;

The above operations cannot underflow due to the check :

require(amount <= supply, "AMOUNT TOO BIG");

Because globalSupply only get increase/decreased when supplies[market] changes, the globalSupply can underflow if supplies[market] doesn't underflow.

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

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

There are 22 instances of this issue:

File: src/DBR.sol

172     balances[msg.sender] -= amount;
174     balances[to] += amount;
196     balances[from] -= amount;
288     dueTokensAccrued[user] += accrued;
289     totalDueTokensAccrued += accrued;
332     debts[user] += replenishmentCost;
360     _totalSupply += amount;
362     balances[to] += amount;
274     balances[from] -= amount;
376     _totalSupply -= amount;

File: src/Fed.sol 

91      supplies[market] += amount;
92      globalSupply += amount;
110     supplies[market] -= amount;
111     globalSupply -= amount;

File: src/Market.sol

395     debts[borrower] += amount;
397     totalDebt += amount;
534     debts[user] -= amount;
535     totalDebt -= amount;
565     debts[user] += replenishmentCost;
568     totalDebt += replenishmentCost;
599     debts[user] -= repaidDebt;
600     totalDebt -= repaidDebt;

5- Splitting require() statements that uses && saves gas (around 8 gas saved per instance) :

There are 5 instances of this issue :

File: src/Market.sol

75      require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps < 10000, "Invalid liquidation incentive");
162     require(_liquidationFactorBps > 0 && _liquidationFactorBps <= 10000, "Invalid liquidation factor");
173     require(_replenishmentIncentiveBps > 0 && _replenishmentIncentiveBps < 10000, "Invalid replenishment incentive");
184     require(_liquidationIncentiveBps > 0 && _liquidationIncentiveBps + liquidationFeeBps < 10000, "Invalid liquidation incentive");
195     require(_liquidationFeeBps > 0 && _liquidationFeeBps + liquidationIncentiveBps < 10000, "Invalid liquidation fee");