You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
While we try our best to maintain readability in the provided code snippets, some functions have been truncated to highlight the affected portions.
It's important to note that during the implementation of these suggested changes, developers must exercise caution to avoid introducing vulnerabilities. Although the optimizations have been tested prior to these recommendations, it is the responsibility of the developers to conduct thorough testing again.
Code reviews and additional testing are strongly advised to minimize any potential risks associated with the refactoring process.
The developers have done an excellent job of identifying and implementing some of the most evident optimizations like use of Immutables. The code is also well commented out which makes it easy for one to follow along.
However, we have identified additional areas where optimization is possible, some of which may not be immediately apparent.
Note on Gas estimates.
There seems to be an issue with how the command forge test --gas-report --optimize gives results. It seems to be giving varying results for the same tests cases. eg For some functions one run might give an average gas of 2000 while running it for the second time gives 1500 . For this reason giving the exact gas savings from the tests included wasn't feasible.
However for the slot packings the gas can be calculated based on how many slots are being saved.
Tighly pack storage variables/optimize the order of variable declaration(Save 2 SLOTS: 4200 Gas)
The EVM works with 32 byte words. Variables less than 32 bytes can be declared next to each other in storage and this will pack the values together into a single 32 byte storage slot (if the values combined are <= 32 bytes). If the variables packed together are retrieved together in functions we will effectively save ~2000 gas with every subsequent SLOAD for that storage slot. This is due to us incurring a Gwarmaccess (100 gas) versus a Gcoldsload (2100 gas).
function setLenderFee(uint256_fee) external onlyOwner {
if (_fee >5000) revertFeeTooHigh();
lenderFee = _fee;
}
File: /src/Lender.sol
84: function setLenderFee(uint256_fee) external onlyOwner {
85: if (_fee >5000) revertFeeTooHigh();
86: lenderFee = _fee;
87: }
When setting lenderFee we have a conditional check that ensures the value assigned to our state variable is always less than 5000. This means any size from uint16 should be big enough to hold our value
Similar thing with borrowerFee where constraint the value to less than 500
if (_fee >500) revertFeeTooHigh();
borrowerFee = _fee;
As the three state variables are being accessed together, packing would save us a lot of gas
diff --git a/src/Lender.sol b/src/Lender.sol
index d04d14f..a5473c6 100644
--- a/src/Lender.sol+++ b/src/Lender.sol@@ -60,9 +60,9 @@ contract Lender is Ownable {
/// @notice the maximum auction length is 3 days
uint256 public constant MAX_AUCTION_LENGTH = 3 days;
/// @notice the fee taken by the protocol in BIPs
- uint256 public lenderFee = 1000;+ uint64 public lenderFee = 1000;
/// @notice the fee taken by the protocol in BIPs
- uint256 public borrowerFee = 50;+ uint64 public borrowerFee = 50;
/// @notice the address of the fee receiver
address public feeReceiver;
Pack structs by putting data types that can fit together next to each other
As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, this saves gas when writing to storage ~20000 gas
/// @notice the minimum size of the loan (to prevent griefing)
uint256 minLoanSize;
/// @notice the maximum size of the loan (also equal to the balance of the lender)
uint256 poolBalance;
/// @notice the max ratio of loanToken/collateralToken (multiplied by 10**18)
uint256 maxLoanRatio;
/// @notice the length of a refinance auction
uint256 auctionLength;
/// @notice the interest rate per year in BIPs
uint256 interestRate;
/// @notice the outstanding loans this pool has
uint256 outstandingLoans;
}
auctionLength and interestRate can be packed with an address(We can save 2 SLOTS: 4000 Gas)
File: /src/utils/Structs.sol
4: struct Pool {
5: /// @notice address of the lender6: address lender;
7: /// @notice address of the loan token8: address loanToken;
9: /// @notice address of the collateral token10: address collateralToken;
11: /// @notice the minimum size of the loan (to prevent griefing)12: uint256 minLoanSize;
13: /// @notice the maximum size of the loan (also equal to the balance of the lender)14: uint256 poolBalance;
15: /// @notice the max ratio of loanToken/collateralToken (multiplied by 10**18)16: uint256 maxLoanRatio;
17: /// @notice the length of a refinance auction18: uint256 auctionLength;
19: /// @notice the interest rate per year in BIPs20: uint256 interestRate;
21: /// @notice the outstanding loans this pool has22: uint256 outstandingLoans;
23: }
When setting up the pool, the values of auctionLength and interestRate are being constrained as shown below
The biggest value here is 100000 , uint64 should be big enough
diff --git a/src/utils/Structs.sol b/src/utils/Structs.sol
index 42d6fb6..1df1d60 100644
--- a/src/utils/Structs.sol+++ b/src/utils/Structs.sol@@ -2,6 +2,10 @@
pragma solidity ^0.8.19;
struct Pool {
+ /// @notice the length of a refinance auction+ uint64 auctionLength;+ /// @notice the interest rate per year in BIPs+ uint64 interestRate;
/// @notice address of the lender
address lender;
/// @notice address of the loan token
@@ -14,10 +18,6 @@ struct Pool {
uint256 poolBalance;
/// @notice the max ratio of loanToken/collateralToken (multiplied by 10**18)
uint256 maxLoanRatio;
- /// @notice the length of a refinance auction- uint256 auctionLength;- /// @notice the interest rate per year in BIPs- uint256 interestRate;
/// @notice the outstanding loans this pool has
uint256 outstandingLoans;
}
/// @notice the amount of collateral locked in the loan
uint256 collateral;
/// @notice the interest rate of the loan per second (in debt tokens)
uint256 interestRate;
/// @notice the timestamp of the loan start
uint256 startTimestamp;
/// @notice the timestamp of a refinance auction start
uint256 auctionStartTimestamp;
/// @notice the refinance auction length
uint256 auctionLength;
}
Due to how values are being constrained when assigning, we can save 2 SLOTS here
1 SLOT = 2k gas Total Gas Saved: 4K Gas We pack interestRate,startTimestamp and auctionLength in one slot We can actually pack more though. See explanation below.
File: /src/utils/Structs.sol
34: struct Loan {
35: /// @notice address of the lender36: address lender;
37: /// @notice address of the borrower38: address borrower;
39: /// @notice address of the loan token40: address loanToken;
41: /// @notice address of the collateral token42: address collateralToken;
43: /// @notice the amount borrowed44: uint256 debt;
45: /// @notice the amount of collateral locked in the loan46: uint256 collateral;
47: /// @notice the interest rate of the loan per second (in debt tokens)48: uint256 interestRate;
49: /// @notice the timestamp of the loan start50: uint256 startTimestamp;
51: /// @notice the timestamp of a refinance auction start52: uint256 auctionStartTimestamp;
53: /// @notice the refinance auction length54: uint256 auctionLength;
55: }
Before we assign our struct values , we have some validations being done for the values that can be assigned
if (pool.interestRate > loan.interestRate) revertRateTooHigh();
// auction length cannot be shorter than old auction length
if (pool.auctionLength < loan.auctionLength) revertAuctionTooShort();
File: /src/Lender.sol
373: if (pool.interestRate > loan.interestRate) revertRateTooHigh();
374: // auction length cannot be shorter than old auction length375: if (pool.auctionLength < loan.auctionLength) revertAuctionTooShort();
The above means two things, loan.interestRate and loan.auctionLength would always be less or equal to pool.interestRate and pool.auctionLength respectively. This constraint is being enforced whenever we write new values to this struct.
The struct above is being set when we give out a loan
File: /src/Lender.sol
415: // update the loan with the new info416: loans[loanId].lender = pool.lender;
417: loans[loanId].interestRate = pool.interestRate;
418: loans[loanId].startTimestamp =block.timestamp;
419: loans[loanId].auctionStartTimestamp =type(uint256).max;
420: loans[loanId].debt = totalDebt;
Note the value being assigned to loans[loanId].interestRate is the value pool.interestRate which comes from a previous struct POOL
The values pool.interestRate and pool.auctionLength have some max enforced. See previous struct packing finding for more explanation
'Note, p.auctionLength and p.interestRate cannot be greater than MAX_AUCTION_LENGTH and MAX_INTEREST_RATE respectively as this would trigger a revert'
This means the variables from the struct Loan need not to be bigger than those in struct Pool.
We can reduce the size to uint64 and still be safe.
For Timestamps uint64 should also be big enough: For auctionStartTimestamp the value seems to be hard-coded as type(uint256).max so I avoided changing it. If not really required we can switch to uint64 and save 1 more SLOT
struct Loan {
+ /// @notice the timestamp of the loan start+ uint64 startTimestamp;+ /// @notice the interest rate of the loan per second (in debt tokens)+ uint64 interestRate;+ /// @notice the refinance auction length+ uint64 auctionLength;
/// @notice address of the lender
address lender;
/// @notice address of the borrower
@@ -42,16 +48,11 @@ struct Loan {
address collateralToken;
/// @notice the amount borrowed
uint256 debt;
+ /// @notice the timestamp of a refinance auction start+ uint256 auctionStartTimestamp;
/// @notice the amount of collateral locked in the loan
uint256 collateral;
- /// @notice the interest rate of the loan per second (in debt tokens)- uint256 interestRate;- /// @notice the timestamp of the loan start- uint256 startTimestamp;- /// @notice the timestamp of a refinance auction start- uint256 auctionStartTimestamp;- /// @notice the refinance auction length- uint256 auctionLength;+
}
Refactoring the above means we need to do some other changes on the other parts of code that were referencing this variables.
Expensive operations inside a for loop
Reading state variables in a loop might be too expensive if the array length is big
// transfer the collateral tokens from the borrower to the contract
IERC20(loan.collateralToken).transferFrom(
msg.sender,
address(this),
collateral
);
loans.push(loan);
emitBorrowed(
msg.sender,
pool.lender,
loans.length-1,
debt,
collateral,
pool.interestRate,
block.timestamp
);
}
File: /src/Lender.sol
for (uint256 i =0; i < borrows.length; i++) {
// calculate the feesuint256 fees = (debt * borrowerFee) /10000;
// transfer feesIERC20(loan.loanToken).transfer(feeReceiver, fees);
// transfer the loan tokens from the pool to the borrowerIERC20(loan.loanToken).transfer(msg.sender, debt - fees);
// transfer the collateral tokens from the borrower to the contractIERC20(loan.collateralToken).transferFrom(
msg.sender,
address(this),
collateral
);
loans.push(loan);
emitBorrowed(
msg.sender,
pool.lender,
loans.length-1,
debt,
collateral,
pool.interestRate,
block.timestamp
);
}
The above loop reads borrowerFee and feeReceiver repeatedly which is too expensive.
Multiple address mappings can be combined into a single mapping of an address 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.
File: /src/Staking.sol
18: /// @notice mapping of user indexes19: mapping(address=>uint256) public supplyIndex;
21: /// @notice mapping of user balances22: mapping(address=>uint256) public balances;
23: /// @notice mapping of user claimable rewards24: mapping(address=>uint256) public claimable;
-- /// @notice mapping of user indexes- mapping(address => uint256) public supplyIndex;- /// @notice mapping of user balances- mapping(address => uint256) public balances;- /// @notice mapping of user claimable rewards- mapping(address => uint256) public claimable;+ struct User{+ uint256 supplyIndex;+ uint256 balances;+ uint256 claimable;+ }+ mapping (address => User) usersInfo;
Using storage instead of memory for structs/arrays saves gas
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
Multiple accesses of a mapping/array should use a local variable cache
Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time. Help the Optimizer by saving a storage variable's reference instead of repeatedly fetching it
To help the optimizer,declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.
As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.
// if new balance > current balance then transfer the difference from the lender
IERC20(p.loanToken).transferFrom(
p.lender,
address(this),
p.poolBalance - currentBalance
);
} elseif (p.poolBalance < currentBalance) {
// if new balance < current balance then transfer the difference back to the lender
IERC20(p.loanToken).transfer(
p.lender,
currentBalance - p.poolBalance
);
}
emitPoolBalanceUpdated(poolId, p.poolBalance);
if (pools[poolId].lender ==address(0)) {
// if the pool doesn't exist then create it
emitPoolCreated(poolId, p);
} else {
// if the pool does exist then update it
emitPoolUpdated(poolId, p);
}
pools[poolId] = p;
}
Lender.sol.setPool(): Cache pools[poolId] in local storage
The gas benchmarks cannot be relied on as the included tests seems to give different values on every run
Min
Average
Median
Max
Before
201208
219563
225608
225608
After
9324
212134
225478
225478
File: /src/Lender.sol
130: function setPool(Pool calldatap) publicreturns (bytes32poolId) {
144: // you can't change the outstanding loans145: if (p.outstandingLoans != pools[poolId].outstandingLoans)
146: revertPoolConfig();
148: uint256 currentBalance = pools[poolId].poolBalance;
167: if (pools[poolId].lender ==address(0)) {
175: pools[poolId] = p;
176: }
diff --git a/src/Lender.sol b/src/Lender.sol
index d04d14f..d384b39 100644
--- a/src/Lender.sol+++ b/src/Lender.sol@@ -142,10 +142,11 @@ contract Lender is Ownable {
poolId = getPoolId(p.lender, p.loanToken, p.collateralToken);
// you can't change the outstanding loans
- if (p.outstandingLoans != pools[poolId].outstandingLoans)+ Pool storage _pool = pools[poolId];+ if (p.outstandingLoans != _pool.outstandingLoans)
revert PoolConfig();
- uint256 currentBalance = pools[poolId].poolBalance;+ uint256 currentBalance = _pool.poolBalance;
if (p.poolBalance > currentBalance) {
// if new balance > current balance then transfer the difference from the lender
@@ -164,7 +165,7 @@ contract Lender is Ownable {
emit PoolBalanceUpdated(poolId, p.poolBalance);
- if (pools[poolId].lender == address(0)) {+ if (_pool.lender == address(0)) {
// if the pool doesn't exist then create it
emit PoolCreated(poolId, p);
} else {
File: /src/Lender.sol
198: function removeFromPool(bytes32poolId, uint256amount) external {
199: if (pools[poolId].lender !=msg.sender) revertUnauthorized();
200: if (amount ==0) revertPoolConfig();
201: _updatePoolBalance(poolId, pools[poolId].poolBalance - amount);
202: // transfer the loan tokens from the contract to the lender203: IERC20(pools[poolId].loanToken).transfer(msg.sender, amount);
204: }
function removeFromPool(bytes32 poolId, uint256 amount) external {
- if (pools[poolId].lender != msg.sender) revert Unauthorized();+ Pool storage _pools = pools[poolId];+ if (_pools.lender != msg.sender) revert Unauthorized();
if (amount == 0) revert PoolConfig();
- _updatePoolBalance(poolId, pools[poolId].poolBalance - amount);+ _updatePoolBalance(poolId, _pools.poolBalance - amount);
// transfer the loan tokens from the contract to the lender
- IERC20(pools[poolId].loanToken).transfer(msg.sender, amount);+ IERC20(_pools.loanToken).transfer(msg.sender, amount);
}
The gas benchmarks cannot be relied on as the included tests seems to give different values on every run
Min
Average
Median
Max
Before
2107
10665
2582
35392
After
997
9665
1481
34712
File: /src/Lender.sol
function buyLoan(uint256loanId, bytes32poolId) public {
// get the loan info
Loan memory loan = loans[loanId];
// validate the loanif (loan.auctionStartTimestamp ==type(uint256).max)
revertAuctionNotStarted();
if (block.timestamp> loan.auctionStartTimestamp + loan.auctionLength)
revertAuctionEnded();
// calculate the current interest rateuint256 timeElapsed =block.timestamp- loan.auctionStartTimestamp;
uint256 currentAuctionRate = (MAX_INTEREST_RATE * timeElapsed) /
loan.auctionLength;
// validate the rateif (pools[poolId].interestRate > currentAuctionRate) revertRateTooHigh();
// calculate the interest
(uint256lenderInterest, uint256protocolInterest) =_calculateInterest(
loan
);
// reject if the pool is not big enoughuint256 totalDebt = loan.debt + lenderInterest + protocolInterest;
if (pools[poolId].poolBalance < totalDebt) revertPoolTooSmall();
// if they do have a big enough pool then transfer from their pool_updatePoolBalance(poolId, pools[poolId].poolBalance - totalDebt);
pools[poolId].outstandingLoans += totalDebt;
// now update the pool balance of the old lenderbytes32 oldPoolId =getPoolId(
loan.lender,
loan.loanToken,
loan.collateralToken
);
_updatePoolBalance(
oldPoolId,
pools[oldPoolId].poolBalance + loan.debt + lenderInterest
);
pools[oldPoolId].outstandingLoans -= loan.debt;
// transfer the protocol fee to the governanceIERC20(loan.loanToken).transfer(feeReceiver, protocolInterest);
emitRepaid(
loan.borrower,
loan.lender,
loanId,
loan.debt + lenderInterest + protocolInterest,
loan.collateral,
loan.interestRate,
loan.startTimestamp
);
// update the loan with the new info
loans[loanId].lender =msg.sender;
loans[loanId].interestRate = pools[poolId].interestRate;
loans[loanId].startTimestamp =block.timestamp;
loans[loanId].auctionStartTimestamp =type(uint256).max;
loans[loanId].debt = totalDebt;
emitBorrowed(
loan.borrower,
msg.sender,
loanId,
loans[loanId].debt,
loans[loanId].collateral,
pools[poolId].interestRate,
block.timestamp
);
emitLoanBought(loanId);
}
diff --git a/src/Lender.sol b/src/Lender.sol
index d04d14f..28ee496 100644
--- a/src/Lender.sol+++ b/src/Lender.sol@@ -458,36 +458,49 @@ contract Lender is Ownable {
}
}
- /// @notice buy a loan in a refinance auction+ /* /// @notice buy a loan in a refinance auction
/// can be called by anyone but you must have a pool with tokens
/// @param loanId the id of the loan to refinance
- /// @param poolId the pool to accept- function buyLoan(uint256 loanId, bytes32 poolId) public {+ /// @param poolId the pool to accept*/+ function buyLoanHelper(uint loanId) internal returns(uint currentAuctionRate,Loan storage loan){
// get the loan info
- Loan memory loan = loans[loanId];+ loan = loans[loanId];
// validate the loan
+ uint256 _auctionStartTimestamp = loan.auctionStartTimestamp;+ if (_auctionStartTimestamp == type(uint256).max)
revert AuctionNotStarted();
+ uint256 _auctionLength = loan.auctionLength;+ if (block.timestamp > _auctionStartTimestamp + _auctionLength)
revert AuctionEnded();
// calculate the current interest rate
- uint256 timeElapsed = block.timestamp - loan.auctionStartTimestamp;- uint256 currentAuctionRate = (MAX_INTEREST_RATE * timeElapsed) /- loan.auctionLength;+ uint256 timeElapsed = block.timestamp - _auctionStartTimestamp;+ currentAuctionRate = (MAX_INTEREST_RATE * timeElapsed) /+ _auctionLength;
// validate the rate
- if (pools[poolId].interestRate > currentAuctionRate) revert RateTooHigh();+ //+ // return (currentAuctionRate,loan);++ }+ function buyLoan(uint256 loanId, bytes32 poolId) public {+ (uint currentAuctionRate, Loan storage loan) = buyLoanHelper(loanId);++ Pool storage _pools = pools[poolId];++ if (_pools.interestRate > currentAuctionRate) revert RateTooHigh();
// calculate the interest
(uint256 lenderInterest, uint256 protocolInterest) = _calculateInterest(
loan
);
// reject if the pool is not big enough
- uint256 totalDebt = loan.debt + lenderInterest + protocolInterest;- if (pools[poolId].poolBalance < totalDebt) revert PoolTooSmall();+ uint256 _loanDebt = loan.debt;+ uint256 totalDebt = _loanDebt + lenderInterest + protocolInterest;+ uint _poolsBalance = _pools.poolBalance;+ if (_poolsBalance < totalDebt) revert PoolTooSmall();
// if they do have a big enough pool then transfer from their pool
- _updatePoolBalance(poolId, pools[poolId].poolBalance - totalDebt);- pools[poolId].outstandingLoans += totalDebt;+ _updatePoolBalance(poolId, _poolsBalance - totalDebt);+ _pools.outstandingLoans += totalDebt;
// now update the pool balance of the old lender
bytes32 oldPoolId = getPoolId(
@@ -495,11 +508,12 @@ contract Lender is Ownable {
loan.loanToken,
loan.collateralToken
);
+ Pool storage _oldPool = pools[oldPoolId];
_updatePoolBalance(
oldPoolId,
- pools[oldPoolId].poolBalance + loan.debt + lenderInterest+ _oldPool.poolBalance + _loanDebt + lenderInterest
);
- pools[oldPoolId].outstandingLoans -= loan.debt;+ _oldPool.outstandingLoans = _oldPool.outstandingLoans - _loanDebt;
// transfer the protocol fee to the governance
IERC20(loan.loanToken).transfer(feeReceiver, protocolInterest);
@@ -508,26 +522,27 @@ contract Lender is Ownable {
loan.borrower,
loan.lender,
loanId,
- loan.debt + lenderInterest + protocolInterest,+ _loanDebt + lenderInterest + protocolInterest,
loan.collateral,
loan.interestRate,
loan.startTimestamp
);
// update the loan with the new info
- loans[loanId].lender = msg.sender;- loans[loanId].interestRate = pools[poolId].interestRate;- loans[loanId].startTimestamp = block.timestamp;- loans[loanId].auctionStartTimestamp = type(uint256).max;- loans[loanId].debt = totalDebt;+ uint256 _poolsInterestRate = _pools.interestRate;+ loan.lender = msg.sender;+ loan.interestRate = _poolsInterestRate;+ loan.startTimestamp = block.timestamp;+ loan.auctionStartTimestamp = type(uint256).max;+ loan.debt = totalDebt;
emit Borrowed(
loan.borrower,
msg.sender,
loanId,
- loans[loanId].debt,- loans[loanId].collateral,- pools[poolId].interestRate,+ totalDebt,+ loan.collateral,+ _poolsInterestRate,
block.timestamp
);
emit LoanBought(loanId);
x += y costs more gas than x = x + y for state variable
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 see resource
The operation _balance - balance cannot underflow due to the check on Line 65 that ensures that _balance is greater than balance for this operation to be performed
The above operation cannot underflow as it would only be performed if p.poolBalance is greater than currentBalance due to the condition check if (p.poolBalance > currentBalance) { on Line 150
It should be safe to wrap the whole external call
This is the else block of the previous check, The code would only be evaluated if p.poolBalance is less than currentBalance due to the check on Line 157
Optimizing check order for cost efficient function execution
Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.
// transfer the loan tokens from the lender to the contract
IERC20(pools[poolId].loanToken).transferFrom(
msg.sender,
address(this),
amount
);
}
Validate function parameters before making any state reads
File: /src/Lender.sol
182: function addToPool(bytes32poolId, uint256amount) external {
183: if (pools[poolId].lender !=msg.sender) revertUnauthorized();
184: if (amount ==0) revertPoolConfig();
The first check in the above, reads a state variable pools[poolId] and compares it to msg.sender The second check only reads a function parameter comparing it against 0.
If the first check passes but we revert on the second case, the gas spent doing the state read on the first check would be wasted. As it's cheaper to read function parameter, the check should be done first.
diff --git a/src/Lender.sol b/src/Lender.sol
index d04d14f..0c6388d 100644
--- a/src/Lender.sol+++ b/src/Lender.sol@@ -180,8 +180,8 @@ contract Lender is Ownable {
/// @param poolId the id of the pool to add to
/// @param amount the amount to add
function addToPool(bytes32 poolId, uint256 amount) external {
- if (pools[poolId].lender != msg.sender) revert Unauthorized();
if (amount == 0) revert PoolConfig();
+ if (pools[poolId].lender != msg.sender) revert Unauthorized();
_updatePoolBalance(poolId, pools[poolId].poolBalance + amount);
// transfer the loan tokens from the lender to the contract
IERC20(pools[poolId].loanToken).transferFrom(
function updateMaxLoanRatio(bytes32poolId, uint256maxLoanRatio) external {
if (pools[poolId].lender !=msg.sender) revertUnauthorized();
if (maxLoanRatio ==0) revertPoolConfig();
Reorder the checks to validate cheaper variables first
File: /src/Lender.sol
210: function updateMaxLoanRatio(bytes32poolId, uint256maxLoanRatio) external {
211: if (pools[poolId].lender !=msg.sender) revertUnauthorized();
212: if (maxLoanRatio ==0) revertPoolConfig();
maxLoanRatio is a function parameter therefore cheaper to read compared to the state read pools[poolId].lender. In case of a revert on the cheaper check, we want to minimize the gas spent, thus should validate the function parameter first
function updateMaxLoanRatio(bytes32 poolId, uint256 maxLoanRatio) external {
- if (pools[poolId].lender != msg.sender) revert Unauthorized();
if (maxLoanRatio == 0) revert PoolConfig();
+ if (pools[poolId].lender != msg.sender) revert Unauthorized();
pools[poolId].maxLoanRatio = maxLoanRatio;
emit PoolMaxLoanRatioUpdated(poolId, maxLoanRatio);
}
function updateInterestRate(bytes32poolId, uint256interestRate) external {
if (pools[poolId].lender !=msg.sender) revertUnauthorized();
if (interestRate > MAX_INTEREST_RATE) revertPoolConfig();
Validate function parameters before reading from state
File: /src/Lender.sol
221: function updateInterestRate(bytes32poolId, uint256interestRate) external {
222: if (pools[poolId].lender !=msg.sender) revertUnauthorized();
223: if (interestRate > MAX_INTEREST_RATE) revertPoolConfig();
As the first check reads from storage,pools[poolId].lender , The second check is way cheaper as it only involves reading a function parameter and a constant value. If we end up reverting on the second check, the gas spent making the state read in the first check would be wasted. Reorder the checks to have the cheaper check first
function updateInterestRate(bytes32 poolId, uint256 interestRate) external {
- if (pools[poolId].lender != msg.sender) revert Unauthorized();
if (interestRate > MAX_INTEREST_RATE) revert PoolConfig();
+ if (pools[poolId].lender != msg.sender) revert Unauthorized();
pools[poolId].interestRate = interestRate;
emit PoolInterestRateUpdated(poolId, interestRate);
}
Conclusion
It is important to emphasize that the provided recommendations aim to enhance the efficiency of the code without compromising its readability. We understand the value of maintainable and easily understandable code to both developers and auditors.
As you proceed with implementing the suggested optimizations, please exercise caution and be diligent in conducting thorough testing. It is crucial to ensure that the changes are not introducing any new vulnerabilities and that the desired performance improvements are achieved. Review code changes, and perform thorough testing to validate the effectiveness and security of the refactored code.
Should you have any questions or need further assistance, please don't hesitate to reach out.
The text was updated successfully, but these errors were encountered:
Gas Optimizations
Severity
Gas Optimization / Informational
Codebase Optimization Report
Auditor's Disclaimer
While we try our best to maintain readability in the provided code snippets, some functions have been truncated to highlight the affected portions.
It's important to note that during the implementation of these suggested changes, developers must exercise caution to avoid introducing vulnerabilities. Although the optimizations have been tested prior to these recommendations, it is the responsibility of the developers to conduct thorough testing again.
Code reviews and additional testing are strongly advised to minimize any potential risks associated with the refactoring process.
Table Of Contents
feeReceiveroutside the loopfeeReceiveroutside the looppools[poolId]in local storageCodebase impressions
The developers have done an excellent job of identifying and implementing some of the most evident optimizations like use of Immutables. The code is also well commented out which makes it easy for one to follow along.
However, we have identified additional areas where optimization is possible, some of which may not be immediately apparent.
Note on Gas estimates.
There seems to be an issue with how the command
forge test --gas-report --optimizegives results. It seems to be giving varying results for the same tests cases. eg For some functions one run might give an average gas of2000while running it for the second time gives1500. For this reason giving the exact gas savings from the tests included wasn't feasible.However for the slot packings the gas can be calculated based on how many slots are being saved.
Tighly pack storage variables/optimize the order of variable declaration(Save 2 SLOTS: 4200 Gas)
The EVM works with 32 byte words. Variables less than 32 bytes can be declared next to each other in storage and this will pack the values together into a single 32 byte storage slot (if the values combined are <= 32 bytes). If the variables packed together are retrieved together in functions we will effectively save ~2000 gas with every subsequent SLOAD for that storage slot. This is due to us incurring a
Gwarmaccess (100 gas)versus aGcoldsload (2100 gas).2023-07-beedle/src/Lender.sol
Line 63 in 658e046
Reducing the size of
lenderFeeshould be safe given how values are assigned to it. see below2023-07-beedle/src/Lender.sol
Lines 84 to 87 in 658e046
When setting
lenderFeewe have a conditional check that ensures the value assigned to our state variable is always less than 5000. This means any size fromuint16should be big enough to hold our valueSimilar thing with
borrowerFeewhere constraint the value to less than500As the three state variables are being accessed together, packing would save us a lot of gas
Pack structs by putting data types that can fit together next to each other
As the solidity EVM works with 32 bytes, variables less than 32 bytes should be packed inside a struct so that they can be stored in the same slot, this saves gas when writing to storage ~20000 gas
2023-07-beedle/src/utils/Structs.sol
Lines 4 to 23 in 658e046
auctionLength and interestRate can be packed with an address(We can save 2 SLOTS: 4000 Gas)
When setting up the pool, the values of
auctionLengthandinterestRateare being constrained as shown below2023-07-beedle/src/Lender.sol
Lines 132 to 139 in 658e046
Note,
p.auctionLength and p.interestRatecannot be greater thanMAX_AUCTION_LENGTHandMAX_INTEREST_RATErespectively as this would trigger a revertFor
MAX_AUCTION_LENGTH, it's a constant with a value of3 dayswhileMAX_INTEREST_RATEhas a value of100000. See https://github.com/Cyfrin/2023-07-beedle/blob/658e046bda8b010a5b82d2d85e824f3823602d27/src/Lender.sol#L59C49-L61The biggest value here is
100000,uint64should be big enough2023-07-beedle/src/utils/Structs.sol
Lines 34 to 55 in 658e046
Due to how values are being constrained when assigning, we can save 2 SLOTS here
1 SLOT = 2k gasTotal Gas Saved: 4K GasWe pack
interestRate,startTimestamp and auctionLengthin one slot We can actually pack more though. See explanation below.Before we assign our struct values , we have some validations being done for the values that can be assigned
2023-07-beedle/src/Lender.sol
Lines 373 to 375 in 658e046
The above means two things,
loan.interestRate and loan.auctionLengthwould always be less or equal topool.interestRate and pool.auctionLengthrespectively.This constraint is being enforced whenever we write new values to this struct.
The struct above is being set when we give out a loan
2023-07-beedle/src/Lender.sol
Lines 355 to 432 in 658e046
Of interest to us is the following piece of code
Note the value being assigned to
loans[loanId].interestRateis the valuepool.interestRatewhich comes from a previous structPOOLThe values
pool.interestRate and pool.auctionLengthhave some max enforced. See previousstruct packing finding for more explanation'Note,
p.auctionLength and p.interestRatecannot be greater thanMAX_AUCTION_LENGTHandMAX_INTEREST_RATErespectively as this would trigger a revert'This means the variables from the
struct Loanneed not to be bigger than those instruct Pool.We can reduce the size to
uint64and still be safe.For Timestamps
uint64should also be big enough:For
auctionStartTimestampthe value seems to be hard-coded astype(uint256).maxso I avoided changing it. If not really required we can switch to uint64 and save 1 more SLOTstruct Loan { + /// @notice the timestamp of the loan start + uint64 startTimestamp; + /// @notice the interest rate of the loan per second (in debt tokens) + uint64 interestRate; + /// @notice the refinance auction length + uint64 auctionLength; /// @notice address of the lender address lender; /// @notice address of the borrower @@ -42,16 +48,11 @@ struct Loan { address collateralToken; /// @notice the amount borrowed uint256 debt; + /// @notice the timestamp of a refinance auction start + uint256 auctionStartTimestamp; /// @notice the amount of collateral locked in the loan uint256 collateral; - /// @notice the interest rate of the loan per second (in debt tokens) - uint256 interestRate; - /// @notice the timestamp of the loan start - uint256 startTimestamp; - /// @notice the timestamp of a refinance auction start - uint256 auctionStartTimestamp; - /// @notice the refinance auction length - uint256 auctionLength; + }Refactoring the above means we need to do some other changes on the other parts of code that were referencing this variables.
Expensive operations inside a for loop
Reading state variables in a loop might be too expensive if the array length is big
**We also emit an event on every loop
2023-07-beedle/src/Lender.sol
Lines 233 to 286 in 658e046
The above loop reads
borrowerFeeandfeeReceiverrepeatedly which is too expensive.2023-07-beedle/src/Lender.sol
Line 325 in 658e046
Cache
feeReceiveroutside the loop2023-07-beedle/src/Lender.sol
Line 403 in 658e046
Cache
feeReceiveroutside the loopMultiple address mappings can be combined into a single mapping of an address 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.
2023-07-beedle/src/Staking.sol
Lines 18 to 24 in 658e046
To help the optimizer further, we declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.
function updateFor(address recipient) public { update(); - uint256 _supplied = balances[recipient]; + User storage _usersInfo = usersInfo[recipient]; + uint256 _supplied = _usersInfo.balances; if (_supplied > 0) { - uint256 _supplyIndex = supplyIndex[recipient]; - supplyIndex[recipient] = index; + uint256 _supplyIndex = _usersInfo.supplyIndex; + _usersInfo.supplyIndex = index; uint256 _delta = index - _supplyIndex; if (_delta > 0) { uint256 _share = _supplied * _delta / 1e18; - claimable[recipient] += _share; + _usersInfo.claimable += _share; } } else { - supplyIndex[recipient] = index; + _usersInfo.supplyIndex = index; } }Using storage instead of memory for structs/arrays saves gas
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
2023-07-beedle/src/Lender.sol
Lines 116 to 121 in 658e046
function getLoanDebt(uint256 loanId) external view returns (uint256 debt) { - Loan memory loan = loans[loanId]; + Loan storage loan = loans[loanId];2023-07-beedle/src/Lender.sol
Line 467 in 658e046
Multiple accesses of a mapping/array should use a local variable cache
Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time.
Help the Optimizer by saving a storage variable's reference instead of repeatedly fetching it
To help the optimizer,declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.
As an example, instead of repeatedly calling
someMap[someIndex], save its reference like this:SomeStruct storage someStruct = someMap[someIndex]and use it.2023-07-beedle/src/Lender.sol
Lines 130 to 176 in 658e046
Lender.sol.setPool(): Cache
pools[poolId]in local storageThe gas benchmarks cannot be relied on as the included tests seems to give different values on every run
2023-07-beedle/src/Lender.sol
Lines 182 to 192 in 658e046
function addToPool(bytes32 poolId, uint256 amount) external { - if (pools[poolId].lender != msg.sender) revert Unauthorized(); + Pool storage _pools = pools[poolId]; + if (_pools.lender != msg.sender) revert Unauthorized(); if (amount == 0) revert PoolConfig(); - _updatePoolBalance(poolId, pools[poolId].poolBalance + amount); + _updatePoolBalance(poolId, _pools.poolBalance + amount); // transfer the loan tokens from the lender to the contract - IERC20(pools[poolId].loanToken).transferFrom( + IERC20(_pools.loanToken).transferFrom( msg.sender, address(this), amount2023-07-beedle/src/Lender.sol
Lines 198 to 204 in 658e046
function removeFromPool(bytes32 poolId, uint256 amount) external { - if (pools[poolId].lender != msg.sender) revert Unauthorized(); + Pool storage _pools = pools[poolId]; + if (_pools.lender != msg.sender) revert Unauthorized(); if (amount == 0) revert PoolConfig(); - _updatePoolBalance(poolId, pools[poolId].poolBalance - amount); + _updatePoolBalance(poolId, _pools.poolBalance - amount); // transfer the loan tokens from the contract to the lender - IERC20(pools[poolId].loanToken).transfer(msg.sender, amount); + IERC20(_pools.loanToken).transfer(msg.sender, amount); }2023-07-beedle/src/Lender.sol
Lines 210 to 215 in 658e046
function updateMaxLoanRatio(bytes32 poolId, uint256 maxLoanRatio) external { - if (pools[poolId].lender != msg.sender) revert Unauthorized(); + Pool storage _pools = pools[poolId]; + if (_pools.lender != msg.sender) revert Unauthorized(); if (maxLoanRatio == 0) revert PoolConfig(); - pools[poolId].maxLoanRatio = maxLoanRatio; + _pools.maxLoanRatio = maxLoanRatio; emit PoolMaxLoanRatioUpdated(poolId, maxLoanRatio); }2023-07-beedle/src/Lender.sol
Lines 221 to 226 in 658e046
function updateInterestRate(bytes32 poolId, uint256 interestRate) external { - if (pools[poolId].lender != msg.sender) revert Unauthorized(); + Pool storage _pools = pools[poolId]; + if (_pools.lender != msg.sender) revert Unauthorized(); if (interestRate > MAX_INTEREST_RATE) revert PoolConfig(); - pools[poolId].interestRate = interestRate; + _pools.interestRate = interestRate; emit PoolInterestRateUpdated(poolId, interestRate); }Lender.sol.buyLoan(): We can optimize this function by caching and using storage for some variables
To avoid stack too deep error, we introduce an internal function
2023-07-beedle/src/Lender.sol
Lines 465 to 534 in 658e046
The gas benchmarks cannot be relied on as the included tests seems to give different values on every run
x += y costs more gas than x = x + y for state variable
2023-07-beedle/src/Staking.sol
Line 41 in 658e046
function deposit(uint _amount) external { TKN.transferFrom(msg.sender, address(this), _amount); updateFor(msg.sender); - balances[msg.sender] += _amount; + balances[msg.sender] = balances[msg.sender] + _amount; }2023-07-beedle/src/Staking.sol
Line 48 in 658e046
function withdraw(uint _amount) external { updateFor(msg.sender); - balances[msg.sender] -= _amount; + balances[msg.sender] = balances[msg.sender] - _amount; TKN.transfer(msg.sender, _amount); }Using 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
see resource
2023-07-beedle/src/Staking.sol
Line 66 in 658e046
The operation
_balance - balancecannot underflow due to the check on Line 65 that ensures that_balanceis greater thanbalancefor this operation to be performed2023-07-beedle/src/Lender.sol
Line 155 in 658e046
The above operation cannot underflow as it would only be performed if
p.poolBalanceis greater thancurrentBalancedue to the condition checkif (p.poolBalance > currentBalance) {on Line 150It should be safe to wrap the whole external call
2023-07-beedle/src/Lender.sol
Line 161 in 658e046
This is the else block of the previous check, The code would only be evaluated if
p.poolBalanceis less thancurrentBalancedue to the check on Line 157Optimizing check order for cost efficient function execution
Checks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas) in a function that may ultimately revert in the unhappy case.
2023-07-beedle/src/Lender.sol
Lines 182 to 192 in 658e046
Validate function parameters before making any state reads
The first check in the above, reads a state variable
pools[poolId]and compares it tomsg.senderThe second check only reads a function parameter comparing it against 0.If the first check passes but we revert on the second case, the gas spent doing the state read on the first check would be wasted. As it's cheaper to read function parameter, the check should be done first.
2023-07-beedle/src/Lender.sol
Lines 198 to 200 in 658e046
No need to validate state variables if we might end up reverting on a cheaper function parameter
Similar explanation to the previous case, we reorder the checks here to validate function parameters first
function removeFromPool(bytes32 poolId, uint256 amount) external { - if (pools[poolId].lender != msg.sender) revert Unauthorized(); if (amount == 0) revert PoolConfig(); + if (pools[poolId].lender != msg.sender) revert Unauthorized();2023-07-beedle/src/Lender.sol
Lines 210 to 212 in 658e046
Reorder the checks to validate cheaper variables first
maxLoanRatiois a function parameter therefore cheaper to read compared to the state readpools[poolId].lender. In case of a revert on the cheaper check, we want to minimize the gas spent, thus should validate the function parameter firstfunction updateMaxLoanRatio(bytes32 poolId, uint256 maxLoanRatio) external { - if (pools[poolId].lender != msg.sender) revert Unauthorized(); if (maxLoanRatio == 0) revert PoolConfig(); + if (pools[poolId].lender != msg.sender) revert Unauthorized(); pools[poolId].maxLoanRatio = maxLoanRatio; emit PoolMaxLoanRatioUpdated(poolId, maxLoanRatio); }2023-07-beedle/src/Lender.sol
Lines 221 to 223 in 658e046
Validate function parameters before reading from state
As the first check reads from storage,
pools[poolId].lender, The second check is way cheaper as it only involves reading a function parameter and a constant value. If we end up reverting on the second check, the gas spent making the state read in the first check would be wasted. Reorder the checks to have the cheaper check firstfunction updateInterestRate(bytes32 poolId, uint256 interestRate) external { - if (pools[poolId].lender != msg.sender) revert Unauthorized(); if (interestRate > MAX_INTEREST_RATE) revert PoolConfig(); + if (pools[poolId].lender != msg.sender) revert Unauthorized(); pools[poolId].interestRate = interestRate; emit PoolInterestRateUpdated(poolId, interestRate); }Conclusion
It is important to emphasize that the provided recommendations aim to enhance the efficiency of the code without compromising its readability. We understand the value of maintainable and easily understandable code to both developers and auditors.
As you proceed with implementing the suggested optimizations, please exercise caution and be diligent in conducting thorough testing. It is crucial to ensure that the changes are not introducing any new vulnerabilities and that the desired performance improvements are achieved. Review code changes, and perform thorough testing to validate the effectiveness and security of the refactored code.
Should you have any questions or need further assistance, please don't hesitate to reach out.
The text was updated successfully, but these errors were encountered: