Platform: Code4rena
Start Date: 12/04/2023
Pot Size: $60,500 USDC
Total HM: 21
Participants: 199
Period: 7 days
Judge: hansfriese
Total Solo HM: 5
Id: 231
League: ETH
Rank: 31/199
Findings: 1
Award: $273.34
š Selected for report: 0
š Solo Findings: 0
š Selected for report: c3phas
Also found by: 0xDACA, 0xRB, 0xSmartContract, 0xhacksmithh, 0xnev, Aymen0909, BenRai, Breeje, DishWasher, Erko, EvanW, JCN, MohammedRizwan, NoamYakov, Polaris_tow, Proxy, Rageur, Raihan, RaymondFam, ReyAdmirado, SAAJ, Sathish9098, Satyam_Sharma, Udsen, __141345__, aria, codeslide, decade, fatherOfBlocks, hunter_w3b, karanctf, matrix_0wl, nadin, naman1778, niser93, pavankv, petrichor, pfapostol, sebghatullah, slvDev, trysam2003, xmxanuel
273.342 USDC - $273.34
| Issue | Instances* | Gas Saved** | |
|---|---|---|---|
| [Gā01] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate | 1 | 10,500 |
| [Gā02] | Avoid contract existence checks by using low level calls | 38 | At least 3800 |
| [Gā03] | Functions guaranteed to revert when called by normal users can be marked payable | 14 | At least 294 |
| [Gā04] | Multiple accesses of a mapping should use a local variable cache | 8 | At least 336 |
| [Gā05] | Multiple accesses of an array should use a local variable cache | 2 | - |
| [Gā06] | State variables should be cached in stack variables rather than re-reading them from storage | 34 | At least 3,400 |
| [Gā07] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement | 16 | At least1360 |
| [Gā08] | The result of internal view function calls should be cached rather than re-calling the function | 1 | - |
Total: 114 instances over 8 issues with at least 19,690 gas saved.
* Exluding any instance from the automated findings (in case the automated findings don't contain all instances).
** Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions. Gas savings are calculated as the sum of all total savings of each public/external function (including any internal/external function call made by this function directly or indirectly) available for the end user. "At least" is written were I didn't calculated the savings of all the instances of an issue on each external call. For those instances which weren't taken into consideration, I calculated the raw savings of the instance - practically assuming that the ineffient code from that instance would be executed (directly or indirectly) on at least one public/external function available for the end user (and isn't dead code).
address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriateSaves 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.
There is 1 instance of this issue:
/// @audit can be a mapping of an address to a struct of address and uint64 83 mapping (address => address) public delegates; 88 mapping (address => uint64) private voteAnchor; // 40 Bit for the block number, 24 Bit sub-block time resolution
Equity.votes(address sender, address[] calldata helpers) (see Equity.canVoteFor(address delegate, address owner)) and Equity.votes(address sender, address[] calldata helpers) (see Equity.votes(address holder))Equity.checkQualified(address sender, address[] calldata helpers)Equity.restructureCapTable(address[] calldata helpers, address[] calldata addressesToWipe)Frankencoin.denyMinter(address _minter, address[] calldata _helpers, string calldata _message)Position.deny(address[] calldata helpers, string calldata message)Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence.
There are 38 instances of this issue:
109 return VALUATION_FACTOR * zchf.equity() * ONE_DEC18 / totalSupply();
243 uint256 equity = zchf.equity();
263 return calculateSharesInternal(zchf.equity(), investment);
279 zchf.transfer(target, proceeds);
292 uint256 capital = zchf.equity();
310 require(zchf.equity() < MINIMUM_EQUITY);
165 success = IERC677Receiver(recipient).onTokenTransfer(msg.sender, amount, data);
93 POSITION_FACTORY.createNewPosition( 94 msg.sender, 95 address(zchf), 96 _collateralAddress, 97 _minCollateral, 98 _mintingMaximum, 99 _initPeriodSeconds, 100 _expirationSeconds, 101 _challengeSeconds, 102 _mintingFeePPM, 103 _liqPrice, 104 _reservePPM 105 )
108 zchf.transferFrom(msg.sender, address(zchf.reserve()), OPENING_FEE);
110 IERC20(_collateralAddress).transferFrom(msg.sender, address(pos), _initialCollateral);
116 require(zchf.isPosition(position) == address(this), "not our pos");
126 uint256 limit = existing.reduceLimitForClone(_initialMint);
127 address pos = POSITION_FACTORY.clonePosition(position);
129 existing.collateral().transferFrom(msg.sender, address(pos), _initialCollateral);
129 existing.collateral().transferFrom(msg.sender, address(pos), _initialCollateral);
142 IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);
142 IERC20(position.collateral()).transferFrom(msg.sender, address(this), _collateralAmount);
144 challenges.push(Challenge(msg.sender, position, _collateralAmount, block.timestamp + position.challengePeriod(), address(0x0), 0));
170 uint256 min = IPosition(challenge.position).minimumCollateral();
204 zchf.transfer(challenge.bidder, challenge.bid); // return old bid
210 zchf.transferFrom(msg.sender, challenge.challenger, _bidAmountZCHF);
225 zchf.transferFrom(msg.sender, address(this), _bidAmountZCHF);
263 IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);
268 zchf.transfer(owner, effectiveBid - fundsNeeded);
272 zchf.transfer(challenge.challenger, reward); // pay out the challenger reward
284 IERC20(collateral).transfer(target, amount);
111 IReserve(zchf.reserve()).checkQualified(msg.sender, helpers);
138 collateral.transferFrom(msg.sender, address(this), newCollateral - colbal);
142 zchf.burnFrom(msg.sender, minted - newMinted, reserveContribution);
170 return IERC20(collateral).balanceOf(address(this));
228 IERC20(zchf).transferFrom(msg.sender, address(this), amount);
233 uint256 actuallyBurned = IFrankencoin(zchf).burnWithReserve(burnable, reserveContribution);
253 IERC20(token).transfer(target, amount);
269 IERC20(collateral).transfer(target, amount);
32 Position clone = Position(createClone(existing.original()));
contracts\StablecoinBridge.sol
45 chf.transferFrom(msg.sender, address(this), amount);
contracts\StablecoinBridge.sol
51 require(chf.balanceOf(address(this)) <= limit, "limit");
contracts\StablecoinBridge.sol
69 chf.transfer(target, amount);
payableIf a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are
CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.
There are 14 instances of this issue:
241 function onTokenTransfer(address from, uint256 amount, bytes calldata) external returns (bool) { 242 require(msg.sender == address(zchf), "caller must be zchf");
172 function mint(address _target, uint256 _amount) override external minterOnly {
262 function burn(address _owner, uint256 _amount) override external minterOnly {
76 function initializeClone(address owner, uint256 _price, uint256 _limit, uint256 _coll, uint256 _mint) external onlyHub {
97 function reduceLimitForClone(uint256 _minimum) external noChallenge noCooldown alive onlyHub returns (uint256) {
132 function adjust(uint256 newMinted, uint256 newCollateral, uint256 newPrice) public onlyOwner {
159 function adjustPrice(uint256 newPrice) public onlyOwner noChallenge {
177 function mint(address target, uint256 amount) public onlyOwner noChallenge noCooldown alive {
227 function repay(uint256 amount) public onlyOwner {
249 function withdraw(address token, address target, uint256 amount) external onlyOwner {
263 function withdrawCollateral(address target, uint256 amount) public onlyOwner noChallenge noCooldown {
292 function notifyChallengeStarted(uint256 size) external onlyHub {
304 function tryAvertChallenge(uint256 _collateralAmount, uint256 _bidAmountZCHF) external onlyHub returns (bool) {
329 function notifyChallengeSucceeded(address _bidder, uint256 _bid, uint256 _size) external onlyHub returns (address, uint256, uint256, uint256, uint32) {
The instances below point to the second+ access of a value inside a mapping, within a function. 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 recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.
There are 8 instances of this issue:
/// @audit _balances[sender] on line 155 155 if (_balances[sender] < amount) revert ERC20InsufficientBalance(sender, _balances[sender], amount);
/// @audit _balances[sender] on line 155 156 _balances[sender] -= amount;
/// @audit minters[_minter] on line 86 88 minters[_minter] = block.timestamp + _applicationPeriod;
/// @audit minters[_minter] on line 153 155 delete minters[_minter];
/// @audit minters[_minter] on line 294 294 return minters[_minter] != 0 && block.timestamp >= minters[_minter];
/// @audit pendingReturns[collateral] on line 282 283 delete pendingReturns[collateral][msg.sender];
/// @audit pendingReturns[collateral][msg.sender] on line 282 283 delete pendingReturns[collateral][msg.sender];
/// @audit pendingReturns[collateral] on line 282 283 delete pendingReturns[collateral][msg.sender];
The instances below point to the second+ access of a value inside an array, within a function. Caching an array's element avoids recalculating the array offsets into memory/calldata and avoids the boundaries check.
There are 2 instances of this issue:
/// @audit challenges[_challengeNumber] on line 200 213 delete challenges[_challengeNumber];
/// @audit challenges[_challengeNumber] on line 253 275 delete challenges[_challengeNumber];
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces 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.
There are 34 instances of this issue:
/// @audit _balances[sender] on line 155 155 if (_balances[sender] < amount) revert ERC20InsufficientBalance(sender, _balances[sender], amount);
/// @audit _balances[sender] on line 155 156 _balances[sender] -= amount;
/// @audit minters[_minter] on line 294 294 return minters[_minter] != 0 && block.timestamp >= minters[_minter];
/// @audit challenge.challenger on line 158 160 challenge.challenger,
/// @audit challenge.bid on line 165 167 challenge.bid -= copy.bid;
/// @audit challenge.size on line 165 168 challenge.size -= copy.size;
/// @audit challenge.position on line 161 170 uint256 min = IPosition(challenge.position).minimumCollateral();
/// @audit challenge.size on line 168 171 require(challenge.size >= min);
/// @audit challenge.challenger on line 158 176 emit ChallengeStarted(challenge.challenger, address(challenge.position), challenge.size, _challengeNumber);
/// @audit challenge.position on line 161 176 emit ChallengeStarted(challenge.challenger, address(challenge.position), challenge.size, _challengeNumber);
/// @audit challenge.size on line 168 176 emit ChallengeStarted(challenge.challenger, address(challenge.position), challenge.size, _challengeNumber);
/// @audit challenge.bid on line 203 204 zchf.transfer(challenge.bidder, challenge.bid); // return old bid
/// @audit challenge.size on line 202 208 if (challenge.position.tryAvertChallenge(challenge.size, _bidAmountZCHF)) {
/// @audit challenge.position on line 208 211 challenge.position.collateral().transfer(msg.sender, challenge.size);
/// @audit challenge.size on line 202 211 challenge.position.collateral().transfer(msg.sender, challenge.size);
/// @audit challenge.position on line 208 212 emit ChallengeAverted(address(challenge.position), _challengeNumber);
/// @audit challenge.end on line 201 218 if (earliestEnd >= challenge.end) {
/// @audit challenge.bidder on line 259 259 address recipient = challenge.bidder == address(0x0) ? msg.sender : challenge.bidder;
/// @audit challenge.bid on line 260 261 if (effectiveBid < challenge.bid) {
/// @audit challenge.bidder on line 259 263 IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);
/// @audit challenge.bid on line 260 263 IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);
/// @audit challenge.challenger on line 254 272 zchf.transfer(challenge.challenger, reward); // pay out the challenger reward
/// @audit challenge.position on line 260 273 emit ChallengeSucceeded(address(challenge.position), challenge.bid, _challengeNumber);
/// @audit challenge.bid on line 260 273 emit ChallengeSucceeded(address(challenge.position), challenge.bid, _challengeNumber);
/// @audit challenge.challenger on line 291 292 emit PostPonedReturn(collateral, challenge.challenger, challenge.size);
/// @audit challenge.size on line 291 292 emit PostPonedReturn(collateral, challenge.challenger, challenge.size);
/// @audit price on line 80 81 if (price > _price) revert InsufficientCollateral();
/// @audit limit on line 98 99 limit -= reduction + _minimum;
/// @audit minted on line 141 142 zchf.burnFrom(msg.sender, minted - newMinted, reserveContribution);
/// @audit minted on line 149 150 mint(msg.sender, newMinted - minted);
/// @audit minted on line 194 196 minted += amount;
/// @audit minted on line 241 241 if (amount > minted) revert RepaidTooMuch(amount - minted);
/// @audit minted on line 241 242 minted -= amount;
/// @audit minted on line 349 349 uint256 repayment = minted < volumeZCHF ? minted : volumeZCHF; // how much must be burned to make things even
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statementrequire(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }.
There are 16 instances of this issue:
/// @audit equity >= amount because of the ternary condition on line 247 247 uint256 shares = equity <= amount ? 1000 * ONE_DEC18 : calculateSharesInternal(equity - amount, amount);
/// @audit currentAllowance >= amount because of the if condition on line 131 132 _approve(sender, msg.sender, currentAllowance - amount);
/// @audit balance >= minReserve because of the if condition on line 141 144 return balance - minReserve;
/// @audit _amount >= usableMint because of the calculation of usableMint on line 166 168 _mint(address(reserve), _amount - usableMint); // rest goes to equity as reserves or as fees
/// @audit _amount >= reserveLeft because of the if condition on line 282 286 _mint(msg.sender, _amount - reserveLeft);
/// @audit xOld >= x because of the ternary condition on line 26 26 cond = xOld > x ? xOld - x > THRESH_DEC18 : x - xOld > THRESH_DEC18;
/// @audit x >= xOld because of the ternary condition on line 26 26 cond = xOld > x ? xOld - x > THRESH_DEC18 : x - xOld > THRESH_DEC18;
/// @audit challenge.bid >= effectiveBid because of the if condition on line 261 263 IERC20(zchf).transfer(challenge.bidder, challenge.bid - effectiveBid);
/// @audit effectiveBid >= fundsNeeded because of the if condition on line 267 268 zchf.transfer(owner, effectiveBid - fundsNeeded);
/// @audit fundsNeeded >= effectiveBid because of the if condition on line 269 270 zchf.notifyLoss(fundsNeeded - effectiveBid); // ensure we have enough to pay everything
/// @audit newCollateral >= colbal because of the if condition on line 137 138 collateral.transferFrom(msg.sender, address(this), newCollateral - colbal);
/// @audit minted >= newMinted because of the if condition on line 141 142 zchf.burnFrom(msg.sender, minted - newMinted, reserveContribution);
/// @audit colbal >= newCollateral because of the if condition on line 145 146 withdrawCollateral(msg.sender, colbal - newCollateral);
/// @audit newMinted >= minted because of the if condition on line 149 150 mint(msg.sender, newMinted - minted);
/// @audit amount >= minted because of the if condition on line 241 241 if (amount > minted) revert RepaidTooMuch(amount - minted);
/// @audit minted >= amount because of the if condition on line 241 242 minted -= amount;
The instances below point to the second+ call of the function within a single function.
There is 1 instance of this issue:
/// @audit minBid(challenge) on line 216 216 if (_bidAmountZCHF < minBid(challenge)) revert BidTooLow(_bidAmountZCHF, minBid(challenge));
#0 - hansfriese
2023-05-16T13:30:40Z
3 is in automated findings
#1 - c4-judge
2023-05-16T13:30:47Z
hansfriese marked the issue as grade-b
#2 - noamyakov
2023-05-18T20:23:36Z
@hansfriese the instances listed under 3 aren't in the automated findings.
* Exluding any instance from the automated findings (in case the automated findings don't contain all instances).
The optimizations suggested in this gas report saves more gas than those suggested in the one that was selected for the report (#956). Please review this gas report again and reconsider your decision for the best gas report.
#3 - c3phas
2023-05-19T13:47:48Z
Saw your reference, Most of the instances on G-02 are also not valid, this is because the project is using compiler version 0.8.13 and the contracts are using a floating pragma ^0.8 meaning they would compile using 0.8.13
#4 - c3phas
2023-05-19T13:54:51Z
Also the judge may miss your comment during QA unless you point him towards it. Just pop on the discussion and inform him you left some comments
#5 - noamyakov
2023-05-19T14:03:11Z
Thank you @c3phas for your feedback.
I'm aware of that, but these optimizations are still valid because the goal is to optimize the project's code as it is. That's how it has always been in C4 competitions.
#6 - hansfriese
2023-05-22T05:57:13Z
After careful review of this report and other reports, this crosses the border of grade-a and one more plus is fewer false positives for this report.
#7 - c4-judge
2023-05-22T05:57:56Z
hansfriese marked the issue as grade-a
#8 - noamyakov
2023-05-23T12:11:36Z
@hansfriese thanks. Have you reconsidered your decision for the best gas report? Do you think report #956 is better?