QA Report

[code423n4]

QA Report

Summary

	Issue	Risk	Instances
1	changeSupplyCeiling should check that _supplyCeiling >= globalSupply when setting a new ceiling	Low	1
2	Change of operator in BorrowController should use two-step transfer pattern	Low	1
3	immutable state variables lack zero value (address or uint) checks	Low	5
4	require()/revert() statements should have descriptive reason strings	NC	1
5	Event should be emitted in setters	NC	8
6	Duplicated require() checks should be refactored to a modifier	NC	7
7	public functions not called by the contract should be declared external instead	NC	40

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.

[c4-judge]

0xean marked the issue as grade-b