Platform: Code4rena
Start Date: 24/03/2023
Pot Size: $49,200 USDC
Total HM: 20
Participants: 246
Period: 6 days
Judge: Picodes
Total Solo HM: 1
Id: 226
League: ETH
Rank: 16/246
Findings: 5
Award: $503.71
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: rbserver
Also found by: 0xAgro, DadeKuma, DeStinE21, HollaDieWaldfee, IgorZuk, J4de, P7N8ZK, Parad0x, Stiglitz, bytes032, carrotsmuggler, csanuragjain, dec3ntraliz3d, kaden, koxuan, lukris02, rvierdiiev, tnevler
58.9366 USDC - $58.94
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L73-L74 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L91 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L115 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L118
It is highly possible that one (or more) of derivatives protocols will be hacked: functions stop working as expected or the assets will be drained. Asymmetry Finance seeks to diversify liquid staking derivatives, which means new derivative contracts will be added, which will only increase this risk. In this case, all funds of all users of the protocol will be locked. Since the stake() and unstake() functions call the functions of all derivative contracts in a loop, and a revert from one derivative will block the execution of these functions.
As stated in the discord chat, derivatives can be removed by setting the weight to 0, but this will not solve the problem, since it is not checked before all calls that the weight is not equal to 0.
For example, 1 out of 3 derivative protocols loses all assets, balance of derivative protocol is 0. Here, in the unstake() function, derivatives[i].balance() returns the amount written in the _balances mapping of derivative protocol (it will be > 0), so derivatives[i].withdraw(derivativeAmount) will be called. But the attempt to withdraw funds will fail, because in fact there are not enough funds on the balance. Unstake() function will constantly revert, no one can return staked funds.
Also, if for some reason the balanceOf() functions or the functions called in .deposit() stop working as expected, then the stake() function may start to malfunction or constantly revert.
Manual review.
Implement a function, which really removes a derivative protocol from the mapping derivatives. For example, put the last derivative info in place of removed derivative, delete data from the last index, and decrease derivativeCount by one.
Also, this function should make an attempt to withdraw funds and, in case of success, reallocate funds to other derivatives, but in case of failure, do not revert, since it is better to lose only part of the funds, and not all the funds placed in other protocols.
#0 - c4-pre-sort
2023-04-04T17:29:12Z
0xSorryNotSorry marked the issue as duplicate of #703
#1 - c4-judge
2023-04-21T15:05:33Z
Picodes marked the issue as satisfactory
🌟 Selected for report: rbserver
Also found by: 0xAgro, DadeKuma, DeStinE21, HollaDieWaldfee, IgorZuk, J4de, P7N8ZK, Parad0x, Stiglitz, bytes032, carrotsmuggler, csanuragjain, dec3ntraliz3d, kaden, koxuan, lukris02, rvierdiiev, tnevler
58.9366 USDC - $58.94
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L71-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L84-L86 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L113-L119 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L140-L143
There is no way to remove a derivative from the derivatives mapping. Although it is possible to set the weight to 0, the loops still make the number of iterations equal to derivativeCount (derivativeCount cannot be decreased). It is also not possible to replace a derivative in the derivatives mapping with another one (for example, if the weight of some derivative was set to 0 and it is no longer used). This can cause out-of-gas error in functions that iterate over all derivatives. For example, the stake() function may be permanently blocked due to the persistent revert().
For example, in stake() function, the functions derivatives[i].ethPerDerivative() (See Note below) and derivatives[i].balance() (twice) are called each time, although some derivatives may no longer be used.
Link:
for (uint i = 0; i < derivativeCount; i++) underlyingValue += (derivatives[i].ethPerDerivative(derivatives[i].balance()) * derivatives[i].balance()) / 10 ** 18;
and here, weight and derivative are read from the storage each time, and only then follows the check (if weight == 0):
for (uint i = 0; i < derivativeCount; i++) { uint256 weight = weights[i]; IDerivative derivative = derivatives[i]; if (weight == 0) continue;
NOTE: derivatives[i].ethPerDerivative() in turn, can make an unlimited number of calls to external contracts. For example, in Reth.sol ethPerDerivative makes in total 10 external calls.
Manual review.
Implement the function removeDerivative(), which deletes a derivative protocol from the mapping derivatives. For example, put the last derivative info in place of removed derivative, delete data from the last index, and decrease derivativeCount by one. Or, implement the function adjustDerivative(), which allows changing derivative contract address.
#0 - c4-pre-sort
2023-04-04T17:32:05Z
0xSorryNotSorry marked the issue as duplicate of #703
#1 - c4-judge
2023-04-21T15:06:32Z
Picodes marked the issue as satisfactory
#2 - c4-judge
2023-04-24T19:36:09Z
Picodes changed the severity to 3 (High Risk)
307.4323 USDC - $307.43
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L115-L116 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/SfrxEth.sol#L74-L75 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L73 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L92
In the ethPerDerivative(), the calculated frxAmount is multiplied by (10 ** 18) and divided by price_oracle, but it must be multiplied by price_oracle and divided by (10 ** 18).
The impact is severe as ethPerDerivative() function is used in stake(), one of two main functions a user will interact with. The value returned by ethPerDerivative() affects the calculations of mintAmount. The incorrect calculation may over or understate the amount of safEth received by the user.
ethPerDerivative() is also used in the withdraw() function when calculating minOut. So, incorrect calculation of ethPerDerivative() may increase/decrease slippage. This can cause unexpected losses or function revert. If withdraw() function reverts, the function unstake() is unavailable => assets are locked.
We need to calculate: (10 ** 18) sfrxEth = X Eth.
For example, we convertToAssets(10 ** 18) and get frxAmount = 1031226769652703996. price_oracle returns 998827832404234820. So, (10 ** 18) frxEth costs 998827832404234820 Eth. Thus, (10 ** 18) sfrxEth costs frxAmount * price_oracle / 10 ** 18 = 1031226769652703996 * 998827832404234820 / 10 ** 18 Eth (1030017999049431492 Eth).
But this function:
function ethPerDerivative(uint256 _amount) public view returns (uint256) { uint256 frxAmount = IsFrxEth(SFRX_ETH_ADDRESS).convertToAssets( 10 ** 18 ); return ((10 ** 18 * frxAmount) / IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle()); }
calculates the cost of sfrxEth as 10 ** 18 * frxAmount / price_oracle = 10 ** 18 * 1031226769652703996 / 998827832404234820 Eth (1032436958800480269 Eth). The current difference ~ 0.23% but it can be more/less.
Manual review.
Change these lines:
return ((10 ** 18 * frxAmount) / IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());
to:
return (frxAmount * IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle() / 10 ** 18);
#0 - c4-pre-sort
2023-04-04T16:48:10Z
0xSorryNotSorry marked the issue as duplicate of #698
#1 - c4-judge
2023-04-21T15:34:33Z
Picodes marked the issue as not a duplicate
#2 - c4-judge
2023-04-21T15:34:38Z
Picodes marked the issue as primary issue
#3 - c4-judge
2023-04-21T15:34:43Z
Picodes marked the issue as satisfactory
#4 - c4-judge
2023-04-21T15:34:47Z
Picodes marked the issue as selected for report
🌟 Selected for report: adriro
Also found by: 0xMirce, 0xRajkumar, 0xepley, BPZ, Bahurum, Bauer, Co0nan, Emmanuel, Franfran, HollaDieWaldfee, IgorZuk, MiloTruck, NoamYakov, RedTiger, Ruhum, T1MOH, Tricko, ad3sh_, auditor0517, bin2chen, carrotsmuggler, eyexploit, handsomegiraffe, igingu, jasonxiale, koxuan, lukris02, monrel, nadin, peanuts, rbserver, rvierdiiev, shaka, sinarette, tnevler, y1cunhui
4.5426 USDC - $4.54
https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/WstEth.sol#L86-L88 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L73 https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/SafEth.sol#L92
function ethPerDerivative() should calculate the price of derivative "in terms of ETH" but actually the function returns the price of WstEth in terms of stEth.
The impact is severe as ethPerDerivative() function is used in stake(), one of two main functions a user will interact with. The value returned by ethPerDerivative() affects the calculations of mintAmount. The incorrect calculation may over or understate the amount of safEth received by the user.
Although the ratio of stEth to eth will be 1:1 after Shanghai, this is not the case at the moment.
The function IWStETH(WST_ETH).getStETHByWstETH() only converts WstEth to stEth. Thus, ethPerDerivative() returns the value of WstEth in terms of stEth, not eth:
function ethPerDerivative(uint256 _amount) public view returns (uint256) { return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18); }
Manual review.
In ethPerDerivative(), add a stEth to eth conversion using the curve pool.
#0 - c4-pre-sort
2023-04-04T17:21:27Z
0xSorryNotSorry marked the issue as duplicate of #588
#1 - c4-judge
2023-04-21T17:09:14Z
Picodes marked the issue as satisfactory
#2 - c4-judge
2023-04-22T09:03:59Z
Picodes marked the issue as partial-50
#3 - c4-judge
2023-04-24T20:45:30Z
Picodes marked the issue as satisfactory
🌟 Selected for report: brgltd
Also found by: 0x3b, 0xAgro, 0xGusMcCrae, 0xNorman, 0xRajkumar, 0xSmartContract, 0xTraub, 0xWagmi, 0xWaitress, 0xffchain, 0xhacksmithh, 0xkazim, 0xnev, 3dgeville, ArbitraryExecution, Aymen0909, BRONZEDISC, Bason, Bloqarl, BlueAlder, Brenzee, CodeFoxInc, CodingNameKiki, Cryptor, DadeKuma, DevABDee, Diana, Dug, Englave, Gde, Haipls, HollaDieWaldfee, Ignite, Infect3d, Jerry0x, Josiah, Kaysoft, Koko1912, KrisApostolov, Lavishq, LeoGold, Madalad, PNS, Rappie, RaymondFam, RedTiger, Rickard, Rolezn, Sathish9098, SunSec, T1MOH, UdarTeam, Udsen, Viktor_Cortess, Wander, adriro, ak1, alejandrocovrr, alexzoid, arialblack14, ayden, bin2chen, brevis, btk, c3phas, carlitox477, catellatech, ch0bu, chaduke, ck, climber2002, codeslide, descharre, dingo2077, ernestognw, fatherOfBlocks, favelanky, georgits, helios, hl_, inmarelibero, juancito, ks__xxxxx, lopotras, lukris02, m_Rassska, mahdirostami, maxper, nadin, navinavu, nemveer, p_crypt0, peanuts, pipoca, pixpi, qpzm, rbserver, reassor, roelio, rotcivegaf, scokaf, siddhpurakaran, slvDev, smaul, tnevler, tsvetanovv, turvy_fuzz, vagrant, wen, yac, zzzitron
42.0604 USDC - $42.06
During the audit, 12 low and 12 non-critical issues were found.
| № | Title | Risk Rating | Instance Count |
|---|---|---|---|
| L-1 | Use the two-step-transfer of ownership | Low | 4 |
| L-2 | Critical changes should use two-step procedure | Low | 6 |
| L-3 | Owner can renounce ownership | Low | 4 |
| L-4 | Add a timelock to critical functions | Low | 6 |
| L-5 | Misleading comment about owner | Low | 3 |
| L-6 | Add return parameter in stake() | Low | 1 |
| L-7 | Check mintAmount | Low | 1 |
| L-8 | Check user token balance | Low | 1 |
| L-9 | Add more information in the events | Low | 5 |
| L-10 | Set minMinAmount | Low | 1 |
| L-11 | Make separate events | Low | 2 |
| L-12 | Unused variable in the ethPerDerivative() is misleading | Low | 2 |
| NC-1 | Missing leading underscores | Non-Critical | 4 |
| NC-2 | Order of Functions | Non-Critical | 11 |
| NC-3 | Typo in event name | Non-Critical | 1 |
| NC-4 | Constants may be used | Non-Critical | 17 |
| NC-5 | Make event names more consistent | Non-Critical | 2 |
| NC-6 | Emit events in initialize() functions | Non-Critical | 4 |
| NC-7 | Put deposit() before withdraw() | Non-Critical | 3 |
| NC-8 | Concatenate multiple lines of code | Non-Critical | 11 |
| NC-9 | Incorrect comment | Non-Critical | 1 |
| NC-10 | Make commentss more consistent | Non-Critical | 2 |
| NC-11 | Natspec is incomplete | Non-Critical | 6 |
| NC-12 | Inconsistency when using uint and uint256 | Non-Critical | 2 |
If the owner accidentally transfers ownership to an incorrect address, protected functions may become permanently inaccessible.
Consider using a two-step-transfer of ownership: the current owner would nominate a new owner, and to become the new owner, the nominated account would have to approve the change, so that the address is proven to be valid.
Lack of two-step procedure for critical operations leaves them error-prone.
Consider adding two step procedure on the critical functions.
Openzeppelin's OwnableUpgradeable.sol implements renounceOwnership() function which leaves the contract without an owner and removes any functionality that is only available to them.
Consider reimplementing the function to disable it.
Giving users time to react and adjust to critical changes in protocol provides more guarantees and increases the transparency of the protocol.
Consider adding a timelock.
Comment for the initialize() function says that parameter owner is a "owner of the contract which handles stake/unstake", although in fact the SafEth contract itself must be specified as the owner.
Change the comments to "address of SafEth contract"
stake()Since msg.value is not the same as the received number of tokens, for more convenience and clarity, it is better to add the return parameter in the stake() function.
Add at the end of the function:
return mintAmount;
mintAmountTo prevent cases when the user pays eth but gets nothing, it should be checked that they receive at least 1 SafEth. Such cases are possible if the protocol lowers the minimum amount of Ether for staking in the future.
require(mintAmount > 0, "no tokens");. Or even better, add a parameter so that users can specify the minimum number of tokens they want to receive.
At the beginning of the unstake() function, it is necessary to check that the user has enough tokens on the balance.
require(balanceOf(msg.sender) >= _safEthAmount, "not enough tokens");
Some events are missing important information.
emit WeightChange(_derivativeIndex, _weight); => add totalWeightemit DerivativeAdded(_contractAddress, _weight, derivativeCount); => add totalWeightemit SetMaxSlippage(_derivativeIndex, _slippage); => add previous slippageemit ChangeMinAmount(minAmount); => add previous minAmountemit ChangeMaxAmount(maxAmount); => add previous maxAmountminMinAmountSet the minimum possible minAmount to ensure that the owner cannot set a value that is too small, which could negatively affect the received tokens due to roundings. This will enhance the reliability and security of the protocol.
Add a constant minMinAmount, and in the setMinAmount(), require(_minAmount >= minMinAmount, "too small value");
It is better to make separate events "StakingPaused" and "StakingUnpaused" ("UnstakingPaused" and "UnstakingUnpaused") because the meaning of events will be more obvious, and there will be no need to remember what 0 and 1 means.
ethPerDerivative() is misleading2 out of 3 derivatives do not use the amount parameter in the ethPerDerivative function, while the third derivative does. This can be confusing because, without documentation, it is not immediately obvious that the function calculates the price in Ether for only one unit of the derivative, and not for a given amount.
For clarity, add comments that clearly state what the amount passed in is used for, and that the price is calculated for only one unit.
Private functions should have a leading underscore.
function rethAddress() private view returns (address) {function swapExactInputSingleHop(function poolCanDeposit(uint256 _amount) private view returns (bool) {function poolPrice() private view returns (uint256) {Add leading underscores where needed.
According to Style Guide, ordering helps readers identify which functions they can call and to find the constructor and fallback definitions easier.
Functions should be grouped according to their visibility and ordered:
receive() function should be placed right after constructor:
public functions should be placed after all external:
external and public functions should be placed before private:
Reorder functions where possible.
event WeightChange(uint indexed index, uint weight); => WeightChangedConstants may be used instead for repeating values.
10 ** 18:
uint256 minOut = (stEthBal * (10 ** 18 - maxSlippage)) / 10 ** 18;return IWStETH(WST_ETH).getStETHByWstETH(10 ** 18);uint256 minOut = (((ethPerDerivative(_amount) * _amount) / 10 ** 18) *(10 ** 18 - maxSlippage)) / 10 ** 18;10 ** 18return ((10 ** 18 * frxAmount) /uint256 minOut = ((((rethPerEth * msg.value) / 10 ** 18) *((10 ** 18 - maxSlippage))) / 10 ** 18);RocketTokenRETHInterface(rethAddress()).getEthValue(10 ** 18);else return (poolPrice() * 10 ** 18) / (10 ** 18);10 ** 18;preDepositPrice = 10 ** 18; // initializes with a price of 1else preDepositPrice = (10 ** 18 * underlyingValue) / totalSupply;) * depositAmount) / 10 ** 18;uint256 mintAmount = (totalStakeValueEth * 10 ** 18) / preDepositPrice;500:
Define constant variables for repeated values.
Change to “MinAmountChanged” and “MaxAmountChanged”.
Consider renaming events.
For off-chain monitoring and transparency, consider emitting an event in initialize() function.
deposit() before withdraw()It is unusual and a little confusing that the deposit() function is placed after the withdraw() function.
To make the code more well-structured, place the deposit() function before withdraw().
This complicates and lengthens the code when several small pieces of code are placed on several lines.
If you combine the specified lines with the previous ones, then they will not become too long.
For example, it looks better:
(bool sent, ) = address(msg.sender).call{value: address(this).balance}("");
than
(bool sent, ) = address(msg.sender).call{value: address(this).balance}( "" );
The comment for the adjustWeight() function says that it "Adds new derivative to the index fund", which is not true.
Link:@notice - Adds new derivative to the index fund @dev - Weights are only in regards to each other, total weight changes with this function @dev - If you want exact weights either do the math off chain or reset all existing derivates to the weights you want @dev - Weights are approximate as it will slowly change as people stake @param _derivativeIndex - index of the derivative you want to update the weight @param _weight - new weight for this derivative. */ function adjustWeight(
Change to @notice - Sets new weight for a certain derivative index
To make comments for all functions with the onlyOwner modifier more consistent, change the two comments.
@notice - Sets the max slippage for a certain derivative index => "Owner only function that sets the max slippage for a certain derivative index"@notice - Sets the minimum amount a user is allowed to stake => "Owner only function that sets the minimum amount a user is allowed to stake"NatSpec does not have parameter descriptions for some functions.
function setMaxSlippage(uint256 _slippage) external onlyOwner { add _slippage descriptionfunction withdraw(uint256 _amount) external onlyOwner { add _amount descriptionfunction ethPerDerivative(uint256 _amount) public view returns (uint256) { add _amount descriptionfunction setMaxSlippage(uint256 _slippage) external onlyOwner { add _slippage descriptionfunction ethPerDerivative(uint256 _amount) public view returns (uint256) { add _amount descriptionfunction withdraw(uint256 amount) external onlyOwner { add _amount descriptionSome variables is declared as uint and some as uint256.
There are 63 instances using uint256 and 16 instances using uint.
event ChangeMinAmount(uint256 indexed minAmount);event Staked(address indexed recipient, uint ethIn, uint safEthOut);Stick to one style.
#0 - c4-sponsor
2023-04-10T16:06:32Z
toshiSat marked the issue as sponsor confirmed
#1 - c4-judge
2023-04-24T17:29:04Z
Picodes marked the issue as grade-a
🌟 Selected for report: Rolezn
Also found by: 0x3b, 0xGordita, 0xSmartContract, 0xhacksmithh, 0xnev, 0xpanicError, 4lulz, Angry_Mustache_Man, ArbitraryExecution, Aymen0909, Bason, BlueAlder, EvanW, Franfran, HHK, Haipls, IgorZuk, JCN, KrisApostolov, Madalad, MiksuJak, MiniGlome, RaymondFam, ReyAdmirado, Rickard, Sathish9098, Udsen, adriro, alexzoid, anodaram, arialblack14, c3phas, carlitox477, ch0bu, chaduke, codeslide, d3e4, dicethedev, ernestognw, fatherOfBlocks, georgits, hunter_w3b, inmarelibero, lukris02, mahdirostami, maxper, pavankv, pixpi, rotcivegaf, smaul, tank, tnevler, wen, yac
90.7432 USDC - $90.74
During the audit, 12 gas issues were found.
| № | Title | Instance Count | Saved |
|---|---|---|---|
| G-1 | Cache state variables instead of reading them from storage multiple times | 16 | 1600 |
| G-2 | Use a function parameter instead of a storage variable | 4 | 400 |
| G-3 | Use local variable cache instead of accessing mapping or array multiple times | 7 | 700 |
| G-4 | Break the if-statement Require | 1 | 400 |
| G-5 | Unnecessary loop in adjustWeight() | 1 | 400 |
| G-6 | Unnecessary loop in addDerivative() | 1 | 400 |
| G-7 | Use unchecked blocks for subtractions where underflow is impossible | 1 | 35 |
| G-8 | Implement a function for removing a derivative | 7 | |
| G-9 | Do not call balance() functions twice | 2 | |
| G-10 | Some expressions can be transformed | 2 | |
| G-11 | Do not multiply and divide by the same number | 1 | |
| G-12 | Use uint256(1) and uint256(2) for pausing | 2 |
Memory read is much cheaper than storage read.
This saves ~100.
So, ~100*16 = 1600
Memory read is much cheaper than storage read.
emit ChangeMinAmount(minAmount); use _minAmountemit ChangeMaxAmount(maxAmount); use _maxAmountemit StakingPaused(pauseStaking); use _pauseemit UnstakingPaused(pauseUnstaking); use _pauseThis saves ~100.
So, ~100*4 = 400
It saves gas due to not having to perform the same key’s keccak256 hash and/or offset recalculation.
(derivatives[i].ethPerDerivative(derivatives[i].balance()) * (cache derivatives[i])derivatives[i].balance()) / (cache derivatives[i])derivatives[i].withdraw(derivativeAmount); (cache derivatives[i])derivatives[i].withdraw(derivatives[i].balance()); (cache derivatives[i]) * 2uint256 ethAmount = (ethAmountToRebalance * weights[i]) / (cache weights[i])derivatives[i].deposit{value: ethAmount}(); (cache derivatives[i])This saves ~100.
So, ~100*7 = 700
Break this condition into 2 parts. if (ethAmountToRebalance == 0) break; to do not iterate the loop for no reason.
Change to:
if (ethAmountToRebalance == 0) break; for (uint i = 0; i < derivativeCount; i++) { if (weights[i] == 0) continue; ...
This saves ~400 (if we take derivativeCount = 3).
adjustWeight()There is no need for the loop.
Instead of loop, consider just calculating the difference between previous and new weight. Change to:
if (_weight > oldWeight) totalWeight += _weight; else totalWeight -= _weight;
This saves ~400 (if we take derivativeCount = 3).
addDerivative()There is no need for the loop.
Instead of loop, consider just calculating adding new weight to the totalWeight. Change to:
totalWeight+=_weight;
This saves ~400 (if we take derivativeCount = 3).
In Solidity 0.8+, there’s a default overflow and underflow check on unsigned integers. When an overflow or underflow isn’t possible (after require or if-statement), some gas can be saved by using unchecked blocks.
This saves ~35.
Consider implementing a function for removing a derivative from the derivatives mapping. Some functions will continue to call derivative contracts which are not longer used (when set weight = 0, and all assets are withdrawn). The reason is that many functions loop through the derivativeCount iterations, i.e. all derivatives.
balance() functions twiceThe value returned by derivatives[i].balance() can be cached.
Can be changed to:
uint256 minOut = ((ethPerDerivative(_amount) * _amount * (10 ** 18 - maxSlippage)) / 10 ** 36;
and to:
uint256 minOut = (((rethPerEth * msg.value) * (10 ** 18 - maxSlippage)) / 10 ** 36);
Change to else return poolPrice(); //(poolPrice() * 10 ** 18) / (10 ** 18)
Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past.
#0 - c4-sponsor
2023-04-07T21:51:00Z
toshiSat marked the issue as sponsor confirmed
#1 - c4-judge
2023-04-23T14:56:27Z
Picodes marked the issue as grade-a