Inverse Finance contest - ElKu's results

Lines of code

https://github.com/code-423n4/2022-10-inverse/blob/3e81f0f5908ea99b36e6ab72f13488bbfe622183/src/Market.sol#L531

Vulnerability details

Impact

When a user wants to Repay his borrowed Dola, the protocol only checks if the Dola he wants to repay is less than or equal to his actual debt as per that particular Market. This means he can repay all his loan in one go and leave the protocol with his uncleared deficit.

Proof of Concept

A user repays his debt by calling the repay function.

This function does the following check:

uint debt = debts[user];
require(debt >= amount, "Insufficient debt");

Once the above condition is cleared, the relevant state variables are updated. After this, the onRepay function in DBR.sol is called which accrues the DBR tokens he owes to the protocol.

Once the user has successfully repaid his debt, he can withdraw all his collateral and leave the protocol with his uncleared debt.

Ideally, the protocol shouldn't allow the user to leave with his deficit uncleared.

A POC was created to show this:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "./FiRMTest.sol";
import "../BorrowController.sol";
import "../DBR.sol";
import "../Fed.sol";
import {SimpleERC20Escrow} from "../escrows/SimpleERC20Escrow.sol";
import "../Market.sol";
import "../Oracle.sol";

import "./mocks/ERC20.sol";
import "./mocks/WETH9.sol";
import "./mocks/BorrowContract.sol";
import {EthFeed} from "./mocks/EthFeed.sol";

contract MarketTest is FiRMTest {
    bytes onlyGovUnpause = "Only governance can unpause";
    bytes onlyPauseGuardianOrGov = "Only pause guardian or governance can pause";

    BorrowContract borrowContract;

    function setUp() public {
        initialize(replenishmentPriceBps, collateralFactorBps, replenishmentIncentiveBps, liquidationBonusBps, callOnDepositCallback);

        vm.startPrank(chair);
        fed.expansion(IMarket(address(market)), 1_000_000e18);
        vm.stopPrank();

        borrowContract = new BorrowContract(address(market), payable(address(WETH)));
    }

    function testRepayElku() public {
        gibWeth(user, wethTestAmount);
        gibDBR(user, wethTestAmount);
        vm.startPrank(user);

	emit log("deposits 1 eth");
        deposit(wethTestAmount);  //deposits 1 eth
	emit log_named_decimal_uint("current CreditLimit",market.getCreditLimit(user),18);
	emit log_named_decimal_uint("current WithdrawalLimit",market.getWithdrawalLimit(user),18);

        emit log("Borrowed maximum of what he can borrow");
        uint borrowAmount = getMaxBorrowAmount(wethTestAmount);  //get how much user can borrow
	emit log_named_decimal_uint("MAX borrowAmount",borrowAmount,18);
        market.borrow(borrowAmount);   //borrow all he can borrow
        emit log_named_decimal_uint("current deficit",dbr.deficitOf(user),18);
	emit log_named_decimal_uint("current CreditLimit",market.getCreditLimit(user),18);
	emit log_named_decimal_uint("current WithdrawalLimit",market.getWithdrawalLimit(user),18);
        emit log_named_decimal_uint("user debt at Market",market.debts(user),18);
        emit log_named_decimal_uint("user debt as per DBR",dbr.debts(user),18);
        skip(10000 days);  
        emit log("skip 10000 days");
        emit log_named_decimal_uint("current deficit",dbr.deficitOf(user),18);
	emit log_named_decimal_uint("current CreditLimit",market.getCreditLimit(user),18);
	emit log_named_decimal_uint("current WithdrawalLimit",market.getWithdrawalLimit(user),18);
        emit log_named_decimal_uint("user debt at Market",market.debts(user),18);
        emit log_named_decimal_uint("user debt as per DBR",dbr.debts(user),18);

        uint currentDebt = market.debts(user);
        market.repay(user, currentDebt);
        emit log("User repaid all the debt");
        emit log_named_decimal_uint("current deficit",dbr.deficitOf(user),18);
	emit log_named_decimal_uint("current CreditLimit",market.getCreditLimit(user),18);
	emit log_named_decimal_uint("current WithdrawalLimit",market.getWithdrawalLimit(user),18);
        emit log_named_decimal_uint("user debt at Market",market.debts(user),18);
        emit log_named_decimal_uint("user debt as per DBR",dbr.debts(user),18);
    }
}

The Logs printed out were:

  deposits 1 eth
  current CreditLimit: 1360.000000000000000000
  current WithdrawalLimit: 1.000000000000000000
  Borrowed maximum of what he can borrow
  MAX borrowAmount: 1360.000000000000000000
  current deficit: 0.000000000000000000
  current CreditLimit: 1360.000000000000000000
  current WithdrawalLimit: 0.000000000000000000
  user debt at Market: 1360.000000000000000000
  user debt as per DBR: 1360.000000000000000000
  skip 10000 days
  current deficit: 37259.273972602739726027
  current CreditLimit: 1360.000000000000000000
  current WithdrawalLimit: 0.000000000000000000
  user debt at Market: 1360.000000000000000000
  user debt as per DBR: 1360.000000000000000000
  User repaid all the debt
  current deficit: 37259.273972602739726027
  current CreditLimit: 1360.000000000000000000
  current WithdrawalLimit: 1.000000000000000000
  user debt at Market: 0.000000000000000000
  user debt as per DBR: 0.000000000000000000

You can see that his deficit is around 37000, but his WithdrawalLimit, is still the original 1 eth which he deposited in the beginning.

Tools Used

VS code, Foundry

Recommended Mitigation Steps

Check the deficit of the user before proceeding with the repayment. Deficit should be zero for user to be able to repay his debt.

Low Risk Issues

Summary Of Findings:

Details on Findings:

1. <ins>Missing zero-address checks</ins>

Zero-address checks must be performed before setting important admin addresses. This is especially important in cases where the address cannot be changed once its set to zero.

The following instances were found:

1. setOperator in BorrowController.sol
2. constructor block in BorrowController.sol
3. constructor block in DBR.sol
4. constructor block in Fed.sol
5. changeGov in Fed.sol
6. constructor in Market.sol
7. setGov in Market.sol
8. constructor block in Oracle.sol

2. <ins>The checks performed in the constructor are not consistent with the set functions</ins>

The following instances were noticed:

1. replenishmentPriceBps assigned in constructor in DBR.sol doesn't check if its value is greater than zero. This check is correctly performed in its set function.
2. replenishmentIncentiveBps assigned in constructor in Market.sol doesn't check if its value is greater than zero. This check is correctly performed in its set function.

3. <ins>Some emit statements doesn't emit all relevant values</ins>

The following instances were noticed:

1. claimOperator() method in DBR.sol doesnt emit the old operator. It only emits the new one.
2. borrowInternal method in Market.sol doesn't emit the address to, to which the borrowed Dola is sent.

4. <ins>Reason strings are missing in some require statements</ins>

When a transaction reverts, its important to let the user know the reason behind the revert. Though this is generally done in the codebase, there are few require statements which doesn't have a reason string.

There are 3 instances of this:

File: src/escrows/GovTokenEscrow.sol

Line 67:    require(msg.sender == beneficiary);

File: src/escrows/INVEscrow.sol

Line 91:    require(msg.sender == beneficiary);

File: src/Fed.sol

Line 93:    require(globalSupply <= supplyCeiling);

5. <ins>Incorrect implementation of BorrowController</ins>

BorrowController contract is supposed to control who can borrow from the protocol and who cannot. Until the address of the BorrowController is set via the setBorrowController function, this check will not be performed. Once we do set the address, any user who wants to borrow would need his contractAllowlist mapping to be turned on to true. This check is performed in borrowInternal function.

This is the incorrect way to implement this. Assuming that this contract would be used for blocking a select group of malicious people, what we need is a BlockList and not an AllowList. Which means when we want to block an address we can explicitly set it to true in its mapping in the BorrowController contract. Every other address will have a default value of false in the mapping and will be allowed to borrow from the Market.

6. <ins>Debt's of multiple markets are cumulatively stored in the same mapping in DBR.sol</ins>

Each Market has a debts mapping which tells us how much the user had borrowed via that particular market.

The same debts are also stored in the DBR contract when DBR tokens are minted to the user for his borrow. This debts mapping is also one dimensional just like in the Market contract. Which means that the debts across all markets are saved cumulatively in the same variable.

This exposes the protocol to a certain risk. If one of the collaterals has an issue with its price or if any one of the markets has a particular vulnerability it can affect all the other markets in which the user has borrowed from. Because in the end, all the debts are stored in the same mapping.

So I recommend that a 2 dimensional mapping is used to store the debts. Something like debts[user][market]. This will make sure that even if one market has an issue it wont affect the other markets directly.

Non-Critical Issues

Summary Of Findings:

Details on Findings:

1. <ins>Use a consistent type name (uint256 or uint) for 256 bit unsigned integers</ins>

It's good to keep consistency in the type which you use. Though they are the same, the codebase will look much more professional and nicer if you use either uint or uint256 instead of using both. Who knows, as the Solidity language is evolving, they might introduce new types which might confuse future readers. I would recommend to stick with uint256, as its more explicit.

There are more than 100 instances of this spread out in several files:

1. BorrowController.sol has 1 instance.
2. DBR.sol has 19 instances.
3. Fed.sol has 17 instances.
4. Market.sol has 69 instances.
5. Oracle.sol has 21 instances.
6. GovTokenEscrow.sol has 1 instance.
7. INVEscrow.sol has 8 instances.
8. SimpleERC20Escrow.sol has 1 instance.

2. <ins>Slight correction in naming, brings perfection</ins>

In DBR.sol, the below mapping is named as allowance. It's more right to name it as allowances. The other variables declared together with this one are named in plural.

mapping(address => mapping(address => uint256)) public allowance;

For example: balances, minters, debts, markets etc..

Look at ERC-20 contract for a more official reference.

3. <ins>Wrong type for dola in Market.sol</ins>

In Market.sol, the state variable dola is declared as immutable. Generally we use immutable type for state variables which are assigned in the constructor.

But here, the constant value is assigned when dola is declared. Two things could be done here:

1. Either make dola a constant. Which will save gas.
2. Or keep it as an immutable, but pass its value in the constructor.

4. <ins>Its better to declare events before the modifiers and functions</ins>

As per Solidity Docs - Style Guide, the order of layout within a contract is as follows:

Type declarations State variables Events Modifiers Functions

But this style is not followed in the following contracts:

1. DBR.sol
2. Fed.sol
3. Market.sol
4. Oracle.sol

Gas Optimizations

Summary Of Findings:

Total Average Gas Saved - This simply means after making the mitigations, the forge gas report was compared with non-optimized version and the difference in average gas was added together for all affected functions.

Detailed Report on Gas Optimization Findings:

1. <ins>Optimizations in Market.sol</ins>

In Market.sol, the optimizations were centered around the following facts:

1. predictEscrow method was introduced to save gas when getting the escrow address. It was said that this saves gas compared to reading directly from the mapping. This was found to be not true.
2. Its better to read the escrow address from mapping instead of using getEscrow in withdrawInternal method. As, if the escrow hasn't created yet, the next statements would cause a revert anyway.
3. Use unchecked arithmetic, whenever we are sure that the particular arithmetic operation wont cause an underflow or overflow.
4. A public function which is never accessed from inside the contract should be declared as external to save gas.
5. If a state variable is accessed more than once in a method, its good to cache its value into a memory variable. As reads from memory costs 3 gas while storage reads cost 100 gas.

The following mitigations were performed in Market.sol to test this:

diff --git "a/.\\2022-10-inverse_original\\src\\Market.sol" "b/.\\2022-10-inverse_gas\\src\\Market.sol"
index 9585b85..9c252c3 100644
--- "a/.\\2022-10-inverse_original\\src\\Market.sol"
+++ "b/.\\2022-10-inverse_gas\\src\\Market.sol"
@@ -310,7 +310,7 @@ contract Market {
     @param user Address of the user.
     */
     function getCollateralValue(address user) public view returns (uint) {
-        IEscrow escrow = predictEscrow(user);
+        IEscrow escrow = escrows[user];
         uint collateralBalance = escrow.balance();
         return collateralBalance * oracle.viewPrice(address(collateral), collateralFactorBps) / 1 ether;
     }
@@ -321,7 +321,7 @@ contract Market {
     @param user Address of the user.
     */
     function getCollateralValueInternal(address user) internal returns (uint) {
-        IEscrow escrow = predictEscrow(user);
+        IEscrow escrow = escrows[user];
         uint collateralBalance = escrow.balance();
         return collateralBalance * oracle.getPrice(address(collateral), collateralFactorBps) / 1 ether;
     }
@@ -351,7 +351,7 @@ contract Market {
     @param user Address of the user.
     */
     function getWithdrawalLimitInternal(address user) internal returns (uint) {
-        IEscrow escrow = predictEscrow(user);
+        IEscrow escrow = escrows[user];  //@audit reading from mapping saves gas
         uint collateralBalance = escrow.balance();
         if(collateralBalance == 0) return 0;
         uint debt = debts[user];
@@ -359,7 +359,9 @@ contract Market {
         if(collateralFactorBps == 0) return 0;
         uint minimumCollateral = debt * 1 ether / oracle.getPrice(address(collateral), collateralFactorBps) * 10000 / collateralFactorBps;
         if(collateralBalance <= minimumCollateral) return 0;
-        return collateralBalance - minimumCollateral;
+        unchecked { //@audit this will never underflow because of `if` condition
+            return collateralBalance - minimumCollateral;
+        }
     }
 
     /**
@@ -367,16 +369,20 @@ contract Market {
      The withdrawal limit is how much collateral a user can withdraw before their loan would be underwater.
     @param user Address of the user.
     */
-    function getWithdrawalLimit(address user) public view returns (uint) {
-        IEscrow escrow = predictEscrow(user);
+    //@audit a function which is not called internally can be declared as `external`.
+    function getWithdrawalLimit(address user) external view returns (uint) {  
+        IEscrow escrow = escrows[user];
         uint collateralBalance = escrow.balance();
         if(collateralBalance == 0) return 0;
         uint debt = debts[user];
         if(debt == 0) return collateralBalance;
-        if(collateralFactorBps == 0) return 0;
-        uint minimumCollateral = debt * 1 ether / oracle.viewPrice(address(collateral), collateralFactorBps) * 10000 / collateralFactorBps;
+        uint collateralFactorBpsCached = collateralFactorBps;  //@audit cache state variables which are repeatedly read
+        if(collateralFactorBpsCached == 0) return 0;  //@audit use cache value here and the line below
+        uint minimumCollateral = debt * 1 ether / oracle.viewPrice(address(collateral), collateralFactorBpsCached) * 10000 / collateralFactorBpsCached;  
         if(collateralBalance <= minimumCollateral) return 0;
-        return collateralBalance - minimumCollateral;
+        unchecked { //@audit this will never underflow because of `if` condition
+            return collateralBalance - minimumCollateral;
+        }
     }
 
     /**
@@ -392,11 +398,13 @@ contract Market {
             require(borrowController.borrowAllowed(msg.sender, borrower, amount), "Denied by borrow controller");
         }
         uint credit = getCreditLimitInternal(borrower);
-        debts[borrower] += amount;
-        require(credit >= debts[borrower], "Exceeded credit limit");
+        uint debt = debts[borrower];   //@audit cache state variable
+        debt += amount;  //@audit use cached state variable in arithmetic operation
+        require(credit >= debt, "Exceeded credit limit");  //@audit use cached state variable in comparison
         totalDebt += amount;
         dbr.onBorrow(borrower, amount);
         dola.transfer(to, amount);
+        debts[borrower] = debt;  //@audit write back the new value to state variable. 
         emit Borrow(borrower, amount);
     }
 
@@ -460,7 +468,7 @@ contract Market {
     function withdrawInternal(address from, address to, uint amount) internal {
         uint limit = getWithdrawalLimitInternal(from);
         require(limit >= amount, "Insufficient withdrawal limit");
-        IEscrow escrow = getEscrow(from);
+        IEscrow escrow = escrows[from]; //@audit read from mapping directly
         escrow.pay(to, amount);
         emit Withdraw(from, to, amount);
     }
@@ -562,12 +570,14 @@ contract Market {
         require(deficit >= amount, "Amount > deficit");
         uint replenishmentCost = amount * dbr.replenishmentPriceBps() / 10000;
         uint replenisherReward = replenishmentCost * replenishmentIncentiveBps / 10000;
-        debts[user] += replenishmentCost;
+        uint debt = debts[user];  //@audit cache state variable
+        debt += replenishmentCost;  //@audit use cached state variable for addition
         uint collateralValue = getCollateralValueInternal(user);
-        require(collateralValue >= debts[user], "Exceeded collateral value");
+        require(collateralValue >= debt, "Exceeded collateral value"); //@audit use cached state variable for comparison
         totalDebt += replenishmentCost;
         dbr.onForceReplenish(user, amount);
         dola.transfer(msg.sender, replenisherReward);
+        debts[user] = debt; //@audit write back the new value to state variable
         emit ForceReplenish(user, msg.sender, amount, replenishmentCost, replenisherReward);
     }
 
@@ -596,11 +606,11 @@ contract Market {
         uint price = oracle.getPrice(address(collateral), collateralFactorBps);
         uint liquidatorReward = repaidDebt * 1 ether / price;
         liquidatorReward += liquidatorReward * liquidationIncentiveBps / 10000;
-        debts[user] -= repaidDebt;
+        debts[user] = debt - repaidDebt;  //@audit use already cached value here for subtraction
         totalDebt -= repaidDebt;
         dbr.onRepay(user, repaidDebt);
         dola.transferFrom(msg.sender, address(this), repaidDebt);
-        IEscrow escrow = predictEscrow(user);
+        IEscrow escrow = escrows[user];   //read directly from the mapping
         escrow.pay(msg.sender, liquidatorReward);
         if(liquidationFeeBps > 0) {
             uint liquidationFee = repaidDebt * 1 ether / price * liquidationFeeBps / 10000;

The following methods showed gas saving when pre and post optimized gas reports from forge was compared:

Total Gas Saved = 5506.

2. <ins>Optimizations in DBR.sol</ins>

In DBR.sol, the optimizations were centered around the following facts:

1. Use unchecked arithmetic, whenever we are sure that the particular arithmetic operation wont cause an underflow or overflow.
2. If a state variable is accessed more than once in a method, its good to cache its value into a memory variable. As reads from memory costs 3 gas while storage reads cost 100 gas.
3. Emit cached memory variables instead of storage variables whenever possible.

The following mitigations were performed in DBR.sol to test this:

diff --git "a/.\\2022-10-inverse_original\\src\\DBR.sol" "b/.\\2022-10-inverse_gas\\src\\DBR.sol"
index aab6daf..a9129bc 100644
--- "a/.\\2022-10-inverse_original\\src\\DBR.sol"
+++ "b/.\\2022-10-inverse_gas\\src\\DBR.sol"
@@ -68,10 +68,11 @@ contract DolaBorrowingRights {
     @notice claims the Operator role if set as pending operator.
     */
     function claimOperator() public {
-        require(msg.sender == pendingOperator, "ONLY PENDING OPERATOR");
-        operator = pendingOperator;
+        address pendingOperatorCached = pendingOperator;  //@audit cache pendingOperator which is accessed multiple times 
+        require(msg.sender == pendingOperatorCached, "ONLY PENDING OPERATOR"); //@audit use cached value here
+        operator = pendingOperatorCached; //@audit use cached value here
         pendingOperator = address(0);
-        emit ChangeOperator(operator);
+        emit ChangeOperator(pendingOperatorCached); //@audit use cached value in the emit statement
     }
 
     /**
@@ -107,8 +108,13 @@ contract DolaBorrowingRights {
     @return uint representing the total supply of DBR.
     */
     function totalSupply() public view returns (uint) {
-        if(totalDueTokensAccrued > _totalSupply) return 0;
-        return _totalSupply - totalDueTokensAccrued;
+        // Caching because we assume that mostly total DBR minted will be greater than total DBR accrued.
+        uint totalDueTokensAccruedCached = totalDueTokensAccrued; //@audit cache state variable
+        uint _totalSupplyCached = _totalSupply;   //@audit cache state variable
+        if(totalDueTokensAccruedCached > _totalSupplyCached) return 0;   //@audit use cached value
+        unchecked {   //@audit use unchecked block as the above `if` makes sure it wont underflow
+            return _totalSupplyCached - totalDueTokensAccruedCached;  //@audit use cached value
+        }
     }
 
     /**
@@ -120,8 +126,11 @@ contract DolaBorrowingRights {
     function balanceOf(address user) public view returns (uint) {
         uint debt = debts[user];
         uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
-        if(dueTokensAccrued[user] + accrued > balances[user]) return 0;
-        return balances[user] - dueTokensAccrued[user] - accrued;
+        uint sum = dueTokensAccrued[user] + accrued;  //@audit caches the addition and the state variable in one go.
+        if(sum > balances[user]) return 0; //@audit use cached value here
+        unchecked{  //@audit use unchecked block as the above `if` makes sure it wont underflow
+            return balances[user] - sum;  //@audit subtract cached value here.
+        }
     }
 
     /**
@@ -133,8 +142,12 @@ contract DolaBorrowingRights {
     function deficitOf(address user) public view returns (uint) {
         uint debt = debts[user];
         uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
-        if(dueTokensAccrued[user] + accrued < balances[user]) return 0;
-        return dueTokensAccrued[user] + accrued - balances[user];
+        uint sum = dueTokensAccrued[user] + accrued;  //@audit caches the addition and the state variable in one go.
+        uint balance = balances[user];   //@audit caches the state variable
+        if(sum < balance) return 0;  //@audit use cached value here
+        unchecked {  //@audit use unchecked block as the above `if` makes sure it wont underflow
+            return sum - balance;    //@audit subtract cached value here.
+        }
     }
     
     /**
@@ -283,8 +296,9 @@ contract DolaBorrowingRights {
     */
     function accrueDueTokens(address user) public {
         uint debt = debts[user];
-        if(lastUpdated[user] == block.timestamp) return;
-        uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
+        uint lastUpdatedCached = lastUpdated[user];  //@audit cache state variable
+        if(lastUpdatedCached == block.timestamp) return;  //@audit use caches value
+        uint accrued = (block.timestamp - lastUpdatedCached) * debt / 365 days; //@audit use caches value
         dueTokensAccrued[user] += accrued;
         totalDueTokensAccrued += accrued;
         lastUpdated[user] = block.timestamp;

The following methods showed gas saving when pre and post optimized gas reports from forge was compared:

Gas savings in DBR.sol:

Gas savings in Market.sol:

Total Gas Saved = Gas savings in DBR.sol + Gas savings in Market.sol = 2190 + 1015 = 3205.

3. <ins>Optimizations in Fed.sol</ins>

In Fed.sol, the optimizations were centered around the following facts:

1. Use unchecked arithmetic, whenever we are sure that the particular arithmetic operation wont cause an underflow or overflow.
2. If a state variable is accessed more than once in a method, its good to cache its value into a memory variable. As reads from memory costs 3 gas while storage reads cost 100 gas.

The following mitigations were performed in Fed.sol to test this:

diff --git "a/.\\2022-10-inverse_original\\src\\Fed.sol" "b/.\\2022-10-inverse_gas\\src\\Fed.sol"
index 1e819bb..ea5ecea 100644
--- "a/.\\2022-10-inverse_original\\src\\Fed.sol"
+++ "b/.\\2022-10-inverse_gas\\src\\Fed.sol"
@@ -88,9 +88,13 @@ contract Fed {
         require(dbr.markets(address(market)), "UNSUPPORTED MARKET");
         require(market.borrowPaused() != true, "CANNOT EXPAND PAUSED MARKETS");
         dola.mint(address(market), amount);
-        supplies[market] += amount;
-        globalSupply += amount;
-        require(globalSupply <= supplyCeiling);
+        uint globalSupplyCached = globalSupply;  //@audit cache state variable
+        globalSupplyCached += amount;  //@audit user arithmetic on cached value
+        require(globalSupplyCached <= supplyCeiling); //@audit use require early on as possible. Use cached value.
+        unchecked{  //@audit use unchecked arithmetic as globalSupplyCached would have overflown if the below would overflow.
+            supplies[market] += amount;
+        }
+        globalSupply = globalSupplyCached;  //@audit write back changed value into storage
         emit Expansion(market, amount);
     }
 
@@ -107,8 +111,10 @@ contract Fed {
         require(amount <= supply, "AMOUNT TOO BIG"); // can't burn profits
         market.recall(amount);
         dola.burn(amount);
-        supplies[market] -= amount;
-        globalSupply -= amount;
+        supplies[market] = supply - amount;  //@audit use already cached value
+        unchecked { //@audit use unchecked arithmetic as supplies would have underflown if the below would underflow.
+            globalSupply -= amount;
+        }
         emit Contraction(market, amount);
     }
 
@@ -121,7 +127,9 @@ contract Fed {
         uint marketValue = dola.balanceOf(address(market)) + market.totalDebt();
         uint supply = supplies[market];
         if(supply >= marketValue) return 0;
-        return marketValue - supply;
+        unchecked {     //@audit use unchecked as the above condition makes sure that it will never underflow
+            return marketValue - supply;  
+        }
     }
 
     /**

The following methods showed gas saving when pre and post optimized gas reports from forge was compared:

Gas savings in DBR.sol:

Gas savings in Fed.sol:

Gas savings in Market.sol:

Total Gas Saved = Gas savings in DBR.sol + Gas savings in Fed.sol + Gas savings in Market.sol = 711 + 1167 + 2842= 4720.

4. <ins>Use bytes32 instead of string whenever possible</ins>

For savings character strings, bytes32 could be used instead of string type if the character array has less than 32 characters. This simple test in remix shows that when deploying, 25000 gas could be saved and the execution cost of getter function could be reduced by 295 gas.

There are two instances of this in DBR.sol:

Line 11:    string public name;
Line 12:    string public symbol;

5. <ins>mapping's of similar type could be combined to turn into a mapping to a struct</ins>

DBR.sol has the following three mappings, which maps from user address to their debts, dueTokensAccrued and lastUpdated.

    mapping (address => uint) public debts; // user => debt across all tracked markets
    mapping (address => uint) public dueTokensAccrued; // user => amount of due tokens accrued
    mapping (address => uint) public lastUpdated; // user => last update timestamp

These can be combined into a structure and then used inside a mapping like this:

    struct User{
        uint256 debts;
        uint256 dueTokensAccrued;
	uint256 lastUpdated;
    }
    mapping (address => User) public userInfo; 

This simple remix test shows that we can save 16 gas per access of each struct field.

Conclusions:

The items listed from 1 to 3 were mitigated and exact gas savings are tabulated to show their value. Items 4 and 5 needed more changes in the codebase, so I couldn't find out the exact gas saving, but their worthiness was proven with simple remix tests.

Items 1 to 3: Exact average gas saved = 5506 + 3205 + 4720 = 13431. Items 4 to 5: Approximate average gas saved = 590 + 48 = 638.