Inverse Finance contest - carlitox477's results

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/DBR.sol#L284-L292 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/DBR.sol#L122 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/DBR.sol#L135 https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/DBR.sol#L148

Vulnerability details

Allows updating lastUpdated[user], what would affect next accrueDueTokens calculations, affecting dueTokensAccrued[user], and functions balanceOf, deficitOf and signedBalanceOf if they are called in next blocks

POC

1. Bob calls Market#borrow with a value of 365 * 60 at block.timestamp = 1.
2. This function calls borrowInternal, which calls dbr.onBorrow
3. The debt is taken
4. Bob knows that as long (block.timestamp - lastUpdated[user]) * debt < 365 the accrueDueTokens function won't update his dueTokensAccrued balance, so he program a bot which allows him to extend his debt by calling accrueDueTokens function considering (block.timestamp - lastUpdated[user]) * 365 * 60 < 365 * 24 * 60 * 60 then (block.timestamp - lastUpdated[user]) < 24 * 60. This mean that if a bot do a call in a block which is emited as long as 24 minutes after last call to accrueDueTokens, Bob will extend his borrow time.

Mitigation steps

If (block.timestamp - lastUpdated[user]) * debt / 365 days = 0 skip next lines in function accrueDueTokens. This means:

function accrueDueTokens(address user) public {
    uint debt = debts[user];
    if(lastUpdated[user] == block.timestamp) return;
    uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
    if(accrued != 0){
        dueTokensAccrued[user] += accrued;
        totalDueTokensAccrued += accrued;
        lastUpdated[user] = block.timestamp;
        emit Transfer(user, address(0), accrued);
    } 
}

Lines of code

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

Vulnerability details

Impact

In the current implementation of Oracle.sol#getPrice(), there is no freshness check. This could lead to stale prices being used.

POC

https://github.com/sherlock-audit/2022-08-sentiment-judging/blob/main/002-M/1-report.md https://github.com/code-423n4/2022-04-backd-findings/issues/17

Mitigation steps

Consider adding the missing freshness check for stale price by adding a validPeriod for each token by adding state variable mapping (address => FeedData) public tokensValidPeriods;, then:

function setValidPeriod(address token, uint256 validPeriod) public onlyOperator {
    if(feeds[token] == address(0)) revert(ERROR_NO_DATA_FEED_SET(token));
    tokensValidPeriods[token] = validPeriod;
}

function getPrice(address token, uint collateralFactorBps) external returns (uint) {
    if(fixedPrices[token] > 0) return fixedPrices[token];
    if(feeds[token].feed != IChainlinkFeed(address(0))) {
        // get price from feed
        
        // @audit Modifation
        (uint80 roundID, int256 answer, , uint256 updatedAt, uint80 answeredInRound)  = feeds[token].feed.latestRoundData();
        require(price > 0, "Invalid feed price");
        require(answeredInRound >= roundID, "Stale price");
        require(block.timestamp <= updatedAt + tokensValidPeriods[token], "Stale price");

        // normalize price
        uint8 feedDecimals = feeds[token].feed.decimals();
        uint8 tokenDecimals = feeds[token].tokenDecimals;
        uint8 decimals = 36 - feedDecimals - tokenDecimals;
        uint normalizedPrice = price * (10 ** decimals);
        // potentially store price as today's low
        uint day = block.timestamp / 1 days;
        uint todaysLow = dailyLows[token][day];
        if(todaysLow == 0 || normalizedPrice < todaysLow) {
            dailyLows[token][day] = normalizedPrice;
            todaysLow = normalizedPrice;
            emit RecordDailyLow(token, normalizedPrice);
        }
        // get yesterday's low
        uint yesterdaysLow = dailyLows[token][day - 1];
        // calculate new borrowing power based on collateral factor
        uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000;
        uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
        if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
            uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
            return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;
        }
        return normalizedPrice;

    }
    revert("Price not found");
}

function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
    if(fixedPrices[token] > 0) return fixedPrices[token];
    if(feeds[token].feed != IChainlinkFeed(address(0))) {
        // get price from feed
        
        // @audit Modifation
        (uint80 roundID, int256 answer, , uint256 updatedAt, uint80 answeredInRound)  = feeds[token].feed.latestRoundData();
        require(price > 0, "Invalid feed price");
        require(answeredInRound >= roundID, "Stale price");
        require(block.timestamp <= updatedAt + tokensValidPeriods[token], "Stale price");


        // normalize price
        uint8 feedDecimals = feeds[token].feed.decimals();
        uint8 tokenDecimals = feeds[token].tokenDecimals;
        uint8 decimals = 36 - feedDecimals - tokenDecimals;
        uint normalizedPrice = price * (10 ** decimals);
        uint day = block.timestamp / 1 days;
        // get today's low
        uint todaysLow = dailyLows[token][day];
        // get yesterday's low
        uint yesterdaysLow = dailyLows[token][day - 1];
        // calculate new borrowing power based on collateral factor
        uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000;
        uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
        if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
            uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
            return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;
        }
        return normalizedPrice;

    }
    revert("Price not found");
}

[LOW] Oracle#constructor: Allow setting operator address to zero

This would let unusable next functions:

• setPendingOperator
• setFeed
• setFixedPrice

[LOW] Oracle#setPendingOperator: Allow to set pending operator to current address operator

This can be avoided by a check

Setting the pending operator to current operator would allow to emit a nonsense event ChangeOperator when claimOperator is called

[LOW] BorrowController should set operator in two steps

Good practise, avoid setting it to zero address

[LOW] GovTokenEscrow: beneficiary can be set to zero address

Giving the lack of zero address check, beneficiary can be set to address zero with no way of change it value, leaving delegate function useless.

[Non-critical] GovTokenEscrow: delegate, balance, pay and initialize visibility should be external

Giving they are not used by any other function in the contract the visibility of next function should be external:

• initialize
• pay
• balance

[LOW] INVEscrow#constructor: _xINV can be set to zero address

Giving the lack of zero address check, _xINV can be set to address zero with no way of change it value, leaving initialize; pay; balance; onDeposit and delegate functions useless, leading to contract redeployment.

Add next line to the constructor:

if(_INVX == _INVX(address(0))) revert ERROR_ADDRESS_ZERO_SETTING()

[Non-critical] delegate, balance, pay and initialize visibility should be external

Giving they are not used by any other function in the contract the visibility of next function should be external:

• initialize
• pay
• balance
• onDeposit
• delegate

[LOW] INVEscrow: beneficiary can be set to zero address

Giving the lack of zero address check, beneficiary can be set to address zero with no way of change it value, leaving delegate function useless.

[LOW] SimpleERC20Escrow: _token can be set to zero address

Giving the lack of zero address check, _token can be set to address zero with no way of change it value, leaving pay and balance functions useless, leading to redeployment.

[LOW] BorrowController#constructor: Lack of check to set operator

Contract creator can create a contract with operator set to zero address.

If address zero is set, next functions would be affected:

• allow
• deny

Recommended code:

constructor(address _operator) {
    if(_operator == address(0)) revert ERROR_INVALID_OPERATOR();
    operator = _operator;
}

[LOW] BorrowController#setOperator Lack of zero address check to set operator

Contract operator can set new operator to to zero address.

If address zero is set, next functions would be affected:

• allow
• deny

Recommended code:

function setOperator(address _operator) public onlyOperator {
    if(_operator == address(0)) revert ERROR_INVALID_OPERATOR();
    operator = _operator;
    }

Functions affected:

• allow
• deny

[LOW] BorrowController should provide methods to set operator in two steps

Is a good practise to set important roles values in two steps. So a state variable pendingOperator should be added and this function should be replace for:

modifier onlyPendingOperator {
        if(msg.sender != pendingOperator) revert ERROR_NOT_PENDING_OPERATOR()
        _;
    }

function setPendingOperator(address _pendingOperator) external onlyOperator {
        pendingOperator = _pendingOperator;
    }

function claimOperator(address _pendingOperator) external onlyPendingOperator {
    pendingOperator = address(0);
    operator = msg.sender;
}

[Non-critical] BorrowController: setOperator, allow, deny, borrowAllowed visibility should be external

Giving they are not used by any other function in the contract the visibility of next function should be external:

• setOperator
• allow
• deny
• borrowAllowed

[LOW] DolaBorrowingRights#constructor allows setting operator to address zero

This would leave next function useless:

• setPendingOperator
• setReplenishmentPriceBps
• addMinter
• removeMinter
• addMarket

As a consequence, the contract will need to be redeployed.

Mitigation steps: add next line to the constructor

if (_operator == address(0)) revert ERROR_ADDRESS_ZERO_ASIGNEMENT();

[Non-Critical] DolaBorrowingRights has multiple functions that can emit meaningless event:

This functions are:

• claimOperator: If pendingOperator == operator. This condition should be checked to avoid a meaningless ChangeOperator event emission
• addMinter: If minters[minter_] == true. This condition should be checked to avoid a meaningless AddMinter event emission
• removeMinter: If minters[minter_] == false. This condition should be checked to avoid a meaningless RemoveMinter event emission
• addMarket: If markets[market_] == true. This condition should be checked to avoid a meaningless AddMarket event emission

[Non-critical]DolaBorrowingRights has multiple function that are public and should be external

• setPendingOperator
• setReplenishmentPriceBps
• claimOperator
• addMinter
• removeMinter
• addMarket
• totalSupply
• signedBalanceOf
• approve
• transfer
• transferFrom
• permit
• invalidateNonce
• onBorrow
• onRepay
• onForceReplenish
• burn
• mint

[LOW] DolaBorrowingRights#setReplenishmentPriceBps should check it is lower/equal to 10.000

replenishmentPriceBps seems to be used just to calculate a porcentage for amount in function onForceReplenish:

uint replenishmentCost = amount * replenishmentPriceBps / 10000;

However if replenishmentPriceBps > 10000 the replenishmentCost will be a multiple of amount.

For this reason, setReplenishmentPriceBps should check that newReplenishmentPriceBps_ is lower/equal than 10.000

[LOW] Fed#constructor: should check zero address values.

Oherwise next function may become useless

• changeGov
• changeChair
• expansion
• contraction
• getProfit
• takeProfit

[LOW] #Fed: governance change should be done in two steps

This in order to follow best practises and avoid setting gov to zero address. So, replace changeGov for:

address pendingGov;

function setPendingGov(address _pendigGov) public {
    require(msg.sender == gov, "ONLY GOV");
    pendingGov = _pendigGov;
}

function claimGov() public {
    require(msg.sender == pendingGov, "ONLY GOV");
    gov = msg.sender;
    pendingGov = addres(0);
}

[LOW] Market#setGov shoulf check non zero address assignment

Otherwise every function with onlyGov modifer will become inaccesible.

[LOW] Market: governance change should be done in two steps

This in order to follow best practises and avoid setting gov to zero address. So, replace setGov for:

address pendingGov;

function setPendingGov(address _pendigGov) public {
    require(msg.sender == gov, "ONLY GOV");
    pendingGov = _pendigGov;
}

function claimGov() public {
    require(msg.sender == pendingGov, "ONLY GOV");
    gov = msg.sender;
    pendingGov = addres(0);
}

INVEscrow: declaring xINV as private rather than public can sabe gas

Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table.

Change this line for:

IXINV private immutable xINV;

INVEscrow#pay: token state variable can be cached

This will save gas, replace this function for:

function pay(address recipient, uint amount) public {
    IERC20 _token = token;
    require(msg.sender == market, "ONLY MARKET");
    uint invBalance = _token.balanceOf(address(this));
    if(invBalance < amount) xINV.redeemUnderlying(amount - invBalance); // we do not check return value because next call will fail if this fails anyway
    _token.transfer(recipient, amount);
}

SimpleERC20Escrow#initialize: extra parameter can be omitted

The second parameter is never used, so it can be deleted in order to save gas. Change this line for:

function initialize(IERC20 _token) external {

BorrowController#borrowAllowed: extra parameters can be omitted

The second parameter is never used, so it can be deleted in order to save gas. Change this line for:

function borrowAllowed(address msgSender) external view returns (bool) {

DolaBorrowingRights: name and symbol can be declared as immutable to save gas

This will save gas during contract creation.

DolaBorrowingRights#balanceOf: dueTokensAccrued[user] and balances[user] can be cached

This will save gas in function balanceOf.

function balanceOf(address user) public view returns (uint) {
    uint debt = debts[user];
    uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days; // accrued debt?
    uint256 _dueTokensAccrued = dueTokensAccrued[user];
    uint256 _balance = balances[user];
    if(_dueTokensAccrued+ accrued > _balance) return 0;
    return _balance - _dueTokensAccrued - accrued;
}

Fed#expansion state variable globalSupply can be cached to save gas

To avoid 3 state variable access to globalSupply replace this code for:

_globalSupply = globalSupply + amount;
require(_globalSupply <= supplyCeiling);
globalSupply = _globalSupply;

Fed#contraction can avoid supplies[market] storage access

Replace this line of code for supplies[market] = supply - amount;

Oracle#claimOperator can avoid access to pendingOperator and operator state variables

Change claimOperator to

function claimOperator() public {
    require(msg.sender == pendingOperator, "ONLY PENDING OPERATOR");
    operator = msg.sender;
    pendingOperator = address(0);
    emit ChangeOperator(msg.sender);
}

Oracle#viewPrice can avoid access to fixedPrices[token] and feeds[token]

Change viewPrice to

function viewPrice(address token, uint collateralFactorBps) external view returns (uint) {
    uint256 _fixedPrice= fixedPrices[token] 
    if(_fixedPrice > 0) return _fixedPrice;
    IChainlinkFeed _feed = feeds[token].feed

    if(_feed.feed != IChainlinkFeed(address(0))) {
        // get price from feed
        uint price = _feed.feed.latestAnswer();
        require(price > 0, "Invalid feed price");
        // normalize price
        uint8 feedDecimals = _feed.feed.decimals();
        uint8 tokenDecimals = _feed.tokenDecimals;
        uint8 decimals = 36 - feedDecimals - tokenDecimals;
        uint normalizedPrice = price * (10 ** decimals);
        uint day = block.timestamp / 1 days;
        // get today's low
        uint todaysLow = dailyLows[token][day];
        // get yesterday's low
        uint yesterdaysLow = dailyLows[token][day - 1];
        // calculate new borrowing power based on collateral factor
        uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000;
        uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
        if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
            uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
            return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;
        }
        return normalizedPrice;

    }
    revert("Price not found");
}

Oracle#viewPrice can avoid access to fixedPrices[token] and feeds[token]

Change getPrice to

function getPrice(address token, uint collateralFactorBps) external returns (uint) {
    uint256 _fixedPrice= fixedPrices[token] 
    if(_fixedPrice > 0) return _fixedPrice;
    IChainlinkFeed _feed = feeds[token]
    if(_feed.feed != IChainlinkFeed(address(0))) {
        // get price from feed
        uint price = _feed.feed.latestAnswer();
        require(price > 0, "Invalid feed price");
        // normalize price
        uint8 feedDecimals = _feed.feed.decimals();
        uint8 tokenDecimals = _feed.tokenDecimals;
        uint8 decimals = 36 - feedDecimals - tokenDecimals;
        uint normalizedPrice = price * (10 ** decimals);
        // potentially store price as today's low
        uint day = block.timestamp / 1 days;
        uint todaysLow = dailyLows[token][day];
        if(todaysLow == 0 || normalizedPrice < todaysLow) {
            dailyLows[token][day] = normalizedPrice;
            todaysLow = normalizedPrice;
            emit RecordDailyLow(token, normalizedPrice);
        }
        // get yesterday's low
        uint yesterdaysLow = dailyLows[token][day - 1];
        // calculate new borrowing power based on collateral factor
        uint newBorrowingPower = normalizedPrice * collateralFactorBps / 10000;
        uint twoDayLow = todaysLow > yesterdaysLow && yesterdaysLow > 0 ? yesterdaysLow : todaysLow;
        if(twoDayLow > 0 && newBorrowingPower > twoDayLow) {
            uint dampenedPrice = twoDayLow * 10000 / collateralFactorBps;
            return dampenedPrice < normalizedPrice ? dampenedPrice: normalizedPrice;
        }
        return normalizedPrice;

    }
    revert("Price not found");
}