Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas Optimizations #344

Open
code423n4 opened this issue Oct 30, 2022 · 1 comment
Open

Gas Optimizations #344

code423n4 opened this issue Oct 30, 2022 · 1 comment
Labels
bug Something isn't working G (Gas Optimization) G-31 grade-b Submission merits a B grade

Comments

@code423n4
Copy link
Contributor

code423n4 commented Oct 30, 2022

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

Instances number of this issue: 26

// src/DBR.sol
174    balances[to] += amount;
198    balances[to] += amount;
288    dueTokensAccrued[user] += accrued;
289    totalDueTokensAccrued += accrued;
304    debts[user] += additionalDebt;
332    debts[user] += replenishmentCost;
360    _totalSupply += amount;
362    balances[to] += amount;

172    balances[msg.sender] -= amount;
196    balances[from] -= amount;
316    debts[user] -= repaidDebt;
374    balances[from] -= amount;
376    _totalSupply -= amount;

// src/Fed.sol
91     supplies[market] += amount;
92     globalSupply += amount;

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


// src/Market.sol
395    debts[borrower] += amount;
397    totalDebt += amount;
565    debts[user] += replenishmentCost;
568    totalDebt += replenishmentCost;
598    liquidatorReward += liquidatorReward * liquidationIncentiveBps / 10000;

534    debts[user] -= amount;
535    totalDebt -= amount;
599    debts[user] -= repaidDebt;
600    totalDebt -= repaidDebt;

Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate

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.

Instances number of this issue: 12

// src/DBR.sol
19    mapping(address => uint256) public balances;
20    mapping(address => mapping(address => uint256)) public allowance;

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

// src/Market.sol
57    mapping (address => IEscrow) public escrows; // user => escrow
58    mapping (address => uint) public debts; // user => debt
59    mapping(address => uint256) public nonces; // user => nonce

// src/Oracle.sol
25    mapping (address => FeedData) public feeds;
26    mapping (address => uint) public fixedPrices;
27    mapping (address => mapping(uint => uint)) public dailyLows; // token => day => price

Boolean comparison

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value.

The demo of the gas comparison can be seen here.

Instances number of this issue: 2

// src/DBR.sol
350        require(minters[msg.sender] == true || msg.sender == operator, "ONLY MINTERS OR OPERATOR");

// src/Fed.sol
89         require(market.borrowPaused() != true, "CANNOT EXPAND PAUSED MARKETS");

Suggestion:
Change to:

// src/DBR.sol
350        require(minters[msg.sender] || msg.sender == operator, "ONLY MINTERS OR OPERATOR");

// src/Fed.sol
89         require(!market.borrowPaused(), "CANNOT EXPAND PAUSED MARKETS");

Add unchecked {} for subtractions where the operands cannot underflow

Some will not underflow because of a previous code or require() or if-statement

And block.timestamp can't be less than lastUpdated[user], due to line 290: lastUpdated[user] = block.timestamp;

Instances number of this issue: 22

// src/DBR.sol
110        if(totalDueTokensAccrued > _totalSupply) return 0;
111        return _totalSupply - totalDueTokensAccrued;

122-124
        uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
        if(dueTokensAccrued[user] + accrued > balances[user]) return 0;
        return balances[user] - dueTokensAccrued[user] - accrued;

135-137
        uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;
* debt / 365 days;
        if(dueTokensAccrued[user] + accrued < balances[user]) return 0;
        return dueTokensAccrued[user] + accrued - balances[user];

148        uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;

287        uint accrued = (block.timestamp - lastUpdated[user]) * debt / 365 days;

171        require(balanceOf(msg.sender) >= amount, "Insufficient balance");
172        balances[msg.sender] -= amount;

195        require(balanceOf(from) >= amount, "Insufficient balance");
196        balances[from] -= amount;

360        _totalSupply += amount;

373        require(balanceOf(from) >= amount, "Insufficient balance");
374        balances[from] -= amount;

// src/Market.sol
532-535
        uint debt = debts[user];
        require(debt >= amount, "Insufficient debt");
        debts[user] -= amount;
        totalDebt -= amount;

// src/Fed.sol
123        if(supply >= marketValue) return 0;
124        return marketValue - supply;

State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.

Instances number of this issue: 4
// src/DBR.sol
totalDueTokensAccrued and _totalSupply:

110        if(totalDueTokensAccrued > _totalSupply) return 0;
111        return _totalSupply - totalDueTokensAccrued;

dueTokensAccrued[user] and balances[user]:

123        if(dueTokensAccrued[user] + accrued > balances[user]) return 0;
124        return balances[user] - dueTokensAccrued[user] - accrued;

// src/Market.sol
escrows[user]:

246        if(escrows[user] != IEscrow(address(0))) return escrows[user];

Functions only called outside of the contract can be declared as external

External call cost is less expensive than of public functions.

The difference is because in public functions, Solidity immediately copies array arguments to memory, while external functions can read directly from calldata. Memory allocation is expensive, whereas reading from calldata is cheap.

Instances number of this issue: 55

// src/BorrowController.sol
    function allow(address allowedContract) public onlyOperator { contractAllowlist[allowedContract] = true; }
    function deny(address deniedContract) public onlyOperator { contractAllowlist[deniedContract] = false; }
    function borrowAllowed(address msgSender, address, uint) public view returns (bool) {

// src/DBR.sol
    function setPendingOperator(address newOperator_) public onlyOperator {
    function setReplenishmentPriceBps(uint newReplenishmentPriceBps_) public onlyOperator {
    function claimOperator() public {
    function addMinter(address minter_) public onlyOperator {
    function removeMinter(address minter_) public onlyOperator {
    function addMarket(address market_) public onlyOperator {
    function approve() public virtual returns (bool) {
    function transfer() public virtual returns (bool) {
    function transferFrom() public virtual returns (bool) {
    function permit() public virtual {
    function invalidateNonce() public {
    function accrueDueTokens(address user) public {
    function onBorrow(address user, uint additionalDebt) public {
    function onRepay(address user, uint repaidDebt) public {
    function onForceReplenish(address user, uint amount) public {
    function burn(uint amount) public {
    function mint(address to, uint amount) public {

// 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 {

// src/Market.sol
    function setOracle(IOracle _oracle) public onlyGov { oracle = _oracle; }
    function setBorrowController(IBorrowController _borrowController) public onlyGov { borrowController = _borrowController; }
    function setGov(address _gov) public onlyGov { gov = _gov; }
    function setLender(address _lender) public onlyGov { lender = _lender; }
    function setPauseGuardian(address _pauseGuardian) public onlyGov { pauseGuardian = _pauseGuardian; }
    function setCollateralFactorBps(uint _collateralFactorBps) public onlyGov {
    function setLiquidationFactorBps(uint _liquidationFactorBps) public onlyGov {
    function setReplenismentIncentiveBps(uint _replenishmentIncentiveBps) public onlyGov {
    function setLiquidationIncentiveBps(uint _liquidationIncentiveBps) public onlyGov {
    function setLiquidationFeeBps(uint _liquidationFeeBps) public onlyGov {
    function recall(uint amount) public {
    function pauseBorrows(bool _value) public {
    function depositAndBorrow(uint amountDeposit, uint amountBorrow) public {
    function borrowOnBehalf(address from, uint amount, uint deadline, uint8 v, bytes32 r, bytes32 s) public {
    function withdrawOnBehalf(address from, uint amount, uint deadline, uint8 v, bytes32 r, bytes32 s) public {
    function invalidateNonce() public {
    function repayAndWithdraw(uint repayAmount, uint withdrawAmount) public {
    function forceReplenish(address user, uint amount) public {
    function liquidate(address user, uint repaidDebt) public {

// src/Oracle.sol
    function setPendingOperator(address newOperator_) public onlyOperator { pendingOperator = newOperator_; }
    function setFeed(address token, IChainlinkFeed feed, uint8 tokenDecimals) public onlyOperator { feeds[token] = FeedData(feed, tokenDecimals); }
    function setFixedPrice(address token, uint price) public onlyOperator { fixedPrices[token] = price; }
    function claimOperator() public {

// src/escrows/GovTokenEscrow.sol
    function pay(address recipient, uint amount) public {
    function delegate(address delegatee) public {

// src/escrows/INVEscrow.sol
    function pay(address recipient, uint amount) public {
    function delegate(address delegatee) public {

// src/escrows/SimpleERC20Escrow.sol
    function pay(address recipient, uint amount) public {

Duplicated functions can be written into a library

invalidateNonce() and DOMAIN_SEPARATOR():

// src/DBR.sol
// src/Market.sol
    function invalidateNonce() public {
        nonces[msg.sender]++;
    }

    function DOMAIN_SEPARATOR() public view virtual returns (bytes32) {
        return block.chainid == INITIAL_CHAIN_ID ? INITIAL_DOMAIN_SEPARATOR : computeDomainSeparator();
    }

Splitting require() statements that use &&

require() statements with multiple conditions can be split.

See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper.

The demo of the gas comparison can be seen here.

Instances number of this issue: 8

// src/DBR.sol
249    require(recoveredAddress != address(0) && recoveredAddress == owner, "INVALID_SIGNER");

// 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");
448    require(recoveredAddress != address(0) && recoveredAddress == from, "INVALID_SIGNER");
512    require(recoveredAddress != address(0) && recoveredAddress == from, "INVALID_SIGNER");

Code reuse

Some functions codes overlap, only a few lines of difference.
Consider reuse the common part and save deployment gas.

  • src/Oracle.sol
    viewPrice() and getPrice()
  • src/Market.sol
    • getCollateralValue(address user) and getCollateralValueInternal(address user)
    • getCreditLimit(address user) and getCreditLimitInternal(address user)
    • getWithdrawalLimitInternal(address user) and getWithdrawalLimit(address user)

Recycle mapping storage

As time goes by, outdated dailyLows[token][day] in src/Oracle.sol will be useless. Consider delete the keys and get some gas refund.

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Oct 30, 2022
code423n4 added a commit that referenced this issue Oct 30, 2022
code423n4 added a commit that referenced this issue Oct 30, 2022
@c4-judge
Copy link
Contributor

c4-judge commented Nov 5, 2022

0xean marked the issue as grade-b

@c4-judge c4-judge added the grade-b Submission merits a B grade label Nov 5, 2022
@C4-Staff C4-Staff added the G-31 label Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working G (Gas Optimization) G-31 grade-b Submission merits a B grade
Projects
None yet
Development

No branches or pull requests

3 participants