Back to Madalad's contests

Asymmetry contest - Madalad's results

A protocol to help diversify and decentralize liquid staking derivatives.

General Information

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

Asymmetry Finance

Findings Distribution

Researcher Performance

Rank: 129/246

Findings: 2

Award: $23.92

QA:
grade-b
Gas:
grade-b

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest PageGithub Contest RepositoryGithub Findings RepositoryAsymmetry Finance

Findings Information

🌟 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

Awards

13.1298 USDC - $13.13

Labels

bug
grade-b
QA (Quality Assurance)
sponsor acknowledged
Q-57

External Links

Link to GitHub issue

Low Summary

IssueInstances
[L-01]Use two-step ownership transfers4
[L-02]Sanitise inputs for critical parameter changes9
[L-03]Use timelock for critical parameter changes8

Total Issues: 3

Total instances: 21

 

Non-Critical Summary

IssueInstances
[N-01]Use indexed for event parameters6
[N-02]Use fixed compiler version4
[N-03]Use appropriate function naming convention4
[N-04]Update import usages31
[N-05]Implementing renounceOwnership is dangerous4
[N-06]Remove unused imports6
[N-07]Missing address(0) checks in constructor/initialize3
[N-08]Use built in constants over magic numbers3
[N-09]Typo in comments1

Total issues: 9

Total instances: 62

 

Low Risk Issues

[L-01] Use two-step ownership transfers

The current ownership transfer process involves the current owner calling transferOwnership(). This function checks the new owner is not the zero address and proceeds to write the new owner's address into the owner's state variable.

If the nominated EOA account is not a valid account, it is entirely possible the owner may accidentally transfer ownership to an uncontrolled account, breaking all functions with the onlyOwner() modifier.

Consider implementing a two step process where the owner nominates an account and the nominated account needs to call an acceptOwnership() function for the transfer of ownership to fully succeed.

This can be achieved using OpenZeppelin's Ownable2Step or Ownable2StepUpgradeable.

Instances: 4

File: contracts/SafEth/derivatives/Reth.sol

19: contract Reth is IDerivative, Initializable, OwnableUpgradeable {

File: contracts/SafEth/derivatives/WstEth.sol

12: contract WstEth is IDerivative, Initializable, OwnableUpgradeable {

File: contracts/SafEth/derivatives/SfrxEth.sol

13: contract SfrxEth is IDerivative, Initializable, OwnableUpgradeable {

File: contracts/SafEth/SafEth.sol

18:     OwnableUpgradeable,

 

[L-02] Sanitise inputs for critical parameter changes

Omitting checks on function parameters for onlyOwner functions that change critical state variables may lead to DoS or lost funds if an erroneous argument is passed.

Instances: 9

File: contracts/SafEth/SafEth.sol

167:        uint256 _weight

183:        address _contractAddress,

184:        uint256 _weight

204:        uint _slippage

214:    function setMinAmount(uint256 _minAmount) external onlyOwner {

223:    function setMaxAmount(uint256 _maxAmount) external onlyOwner {

File: contracts/SafEth/derivatives/Reth.sol

58:    function setMaxSlippage(uint256 _slippage) external onlyOwner {

File: contracts/SafEth/derivatives/SfrxEth.sol

51:    function setMaxSlippage(uint256 _slippage) external onlyOwner {

File: contracts/SafEth/derivatives/WstEth.sol

48:    function setMaxSlippage(uint256 _slippage) external onlyOwner {

 

[L-03] Use timelock for critical parameter changes

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Instances: 8

File: contracts/SafEth/SafEth.sol

165:    function adjustWeight(

182:    function addDerivative(

202:    function setMaxSlippage(

214:    function setMinAmount(uint256 _minAmount) external onlyOwner {

223:    function setMaxAmount(uint256 _maxAmount) external onlyOwner {

File: contracts/SafEth/derivatives/Reth.sol

58:    function setMaxSlippage(uint256 _slippage) external onlyOwner {

File: contracts/SafEth/derivatives/SfrxEth.sol

51:    function setMaxSlippage(uint256 _slippage) external onlyOwner {

File: contracts/SafEth/derivatives/WstEth.sol

48:    function setMaxSlippage(uint256 _slippage) external onlyOwner {

 

 

Non-Critical Issues

[N-01] Use indexed for event parameters

Index event fields make the field more quickly accessible to off-chain tools that parse events.

If the variable is value type (uint, address, bool), then using indexed saves gas. Otherwise, each index field costs extra gas during emission.

Instances: 6

File: contracts/SafEth/SafEth.sol

25:     event SetMaxSlippage(uint256 indexed index, uint256 slippage);

26:     event Staked(address indexed recipient, uint ethIn, uint safEthOut);

27:     event Unstaked(address indexed recipient, uint ethOut, uint safEthIn);

28:     event WeightChange(uint indexed index, uint weight);

31:         uint weight,

32:         uint index

 

[N-02] Use fixed compiler version

A floating pragma risks a different compiler version being used in production vs testing, which poses security risks.

Instances: 4

File: contracts/SafEth/derivatives/Reth.sol

2: pragma solidity ^0.8.13;

File: contracts/SafEth/derivatives/WstEth.sol

2: pragma solidity ^0.8.13;

File: contracts/SafEth/derivatives/SfrxEth.sol

2: pragma solidity ^0.8.13;

File: contracts/SafEth/SafEth.sol

2: pragma solidity ^0.8.13;

 

[N-03] Use appropriate function naming convention

According to Solidity's style guide and popular convention, function names should be prefixed with an underscore if and only if they are private or internal.

See here for more details.

Instances: 4

File: contracts/SafEth/derivatives/Reth.sol

66:     function rethAddress() private view returns (address) {

83:     function swapExactInputSingleHop(

120:     function poolCanDeposit(uint256 _amount) private view returns (bool) {

228:     function poolPrice() private view returns (uint256) {

 

[N-04] Update import usages

To improve readability and adhere to the rule of modularity and modular programming: only import what you need. Specific imports with curly braces allow us to apply this rule better.

For example:

import {ERC721} from "solmate/tokens/ERC721.sol";

Instances: 31

File: contracts/SafEth/derivatives/Reth.sol

4: import "../../interfaces/IDerivative.sol";

5: import "../../interfaces/frax/IsFrxEth.sol";

6: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

7: import "../../interfaces/rocketpool/RocketStorageInterface.sol";

8: import "../../interfaces/rocketpool/RocketTokenRETHInterface.sol";

9: import "../../interfaces/rocketpool/RocketDepositPoolInterface.sol";

10: import "../../interfaces/rocketpool/RocketDAOProtocolSettingsDepositInterface.sol";

11: import "../../interfaces/IWETH.sol";

12: import "../../interfaces/uniswap/ISwapRouter.sol";

13: import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

14: import "../../interfaces/uniswap/IUniswapV3Factory.sol";

15: import "../../interfaces/uniswap/IUniswapV3Pool.sol";

File: contracts/SafEth/derivatives/WstEth.sol

4: import "../../interfaces/IDerivative.sol";

5: import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

6: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

7: import "../../interfaces/curve/IStEthEthPool.sol";

8: import "../../interfaces/lido/IWStETH.sol";

File: contracts/SafEth/derivatives/SfrxEth.sol

4: import "../../interfaces/IDerivative.sol";

5: import "../../interfaces/frax/IsFrxEth.sol";

6: import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

7: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

8: import "../../interfaces/curve/IFrxEthEthPool.sol";

9: import "../../interfaces/frax/IFrxETHMinter.sol";

File: contracts/SafEth/SafEth.sol

4: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

5: import "../interfaces/IWETH.sol";

6: import "../interfaces/uniswap/ISwapRouter.sol";

7: import "../interfaces/lido/IWStETH.sol";

8: import "../interfaces/lido/IstETH.sol";

9: import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";

10: import "./SafEthStorage.sol";

11: import "@openzeppelin/contracts-upgradeable/token/ERC20/ERC20Upgradeable.sol";

 

[N-05] Implementing renounceOwnership is dangerous

Typically, the contract's owner is the account that deploys the contract. As a result, the owner is able to perform certain privileged activities.

The OpenZeppelin's Ownable used in this project contract implements renounceOwnership. This can represent a certain risk if the ownership is renounced for any other reason than by design.

Renouncing ownership will leave the contract without an owner, thereby removing any functionality that is only available to the owner.

It is recommended to either reimplement the function to disable it, or to clearly specify if it is part of the contract design.

Instances: 4

File: contracts/SafEth/derivatives/Reth.sol

19: contract Reth is IDerivative, Initializable, OwnableUpgradeable {

File: contracts/SafEth/derivatives/WstEth.sol

12: contract WstEth is IDerivative, Initializable, OwnableUpgradeable {

File: contracts/SafEth/derivatives/SfrxEth.sol

13: contract SfrxEth is IDerivative, Initializable, OwnableUpgradeable {

File: contracts/SafEth/SafEth.sol

15: contract SafEth is

 

[N-06] Remove unused imports

Instances: 6

File: contracts/SafEth/derivatives/Reth.sol

5: import "../../interfaces/frax/IsFrxEth.sol";

File: contracts/SafEth/SafEth.sol

4: import "@openzeppelin/contracts/token/ERC20/IERC20.sol";

5: import "../interfaces/IWETH.sol";

6: import "../interfaces/uniswap/ISwapRouter.sol";

7: import "../interfaces/lido/IWStETH.sol";

8: import "../interfaces/lido/IstETH.sol";

 

[N-07] Missing address(0) checks in constructor/initialize

Failing to check for invalid parameters on deployment may result in an erroneous input and require an expensive redeployment.

Instances: 3

File: contracts/SafEth/derivatives/Reth.sol

42:     function initialize(address _owner) external initializer {

File: contracts/SafEth/derivatives/WstEth.sol

33:     function initialize(address _owner) external initializer {

File: contracts/SafEth/derivatives/SfrxEth.sol

36:     function initialize(address _owner) external initializer {

 

[N-08] Use built in constants over magic numbers

Improves code readability and reduces margin for error e.g. 200 ether instead of 200 * 10 ** 18.

Instances: 2

File: contracts/SafEth/SafEth.sol

54:        minAmount = 5 * 10 ** 17; // initializing with .5 ETH as minimum

55:        maxAmount = 200 * 10 ** 18; // initializing with 200 ETH as maximum
File: contracts/SafEth/derivatives/Reth.sol

44:        maxSlippage = (1 * 10 ** 16); // 1%

 

[N-09] Typo in comments

Comment for SafEth.adjustWeight is misleading, as it claims that a weight is being added rather than adjusted.

Instances: 1

File: contracts/SafEth/derivatives/Reth.sol

54:        @notice - Adds new derivative to the index fund

#0 - c4-sponsor

2023-04-10T18:20:18Z

toshiSat marked the issue as sponsor acknowledged

#1 - c4-judge

2023-04-24T17:37:16Z

Picodes marked the issue as grade-b

Findings Information

🌟 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

Awards

10.7864 USDC - $10.79

Labels

bug
G (Gas Optimization)
grade-b
sponsor confirmed
G-20

External Links

Link to GitHub issue

Gas Optimizations Summary

IssueInstances
[G-01]Functions guaranteed to revert when called by normal users can be marked payable14
[G-02]Use assembly to calculate hashes6
[G-03]Do not compare boolean expressions to boolean literals2
[G-04]Division/Multiplication by 2 should use bit shifting1
[G-05]Use indexed to save gas6
[G-06]Use unchecked for operations that cannot overflow/underflow7
[G-07]Use private rather than public for constants11
[G-08]Change public functions to external8
[G-09]Use named return values15

Total issues: 9

Total instances: 70

 

Gas Optimizations

[G-01] Functions guaranteed to revert when called by normal users can be marked payable

If 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 (2400 per instance).

Instances: 14

File: contracts/SafEth/derivatives/Reth.sol

58:     function setMaxSlippage(uint256 _slippage) external onlyOwner {

107:     function withdraw(uint256 amount) external onlyOwner {

File: contracts/SafEth/derivatives/WstEth.sol

48:     function setMaxSlippage(uint256 _slippage) external onlyOwner {

56:     function withdraw(uint256 _amount) external onlyOwner {

File: contracts/SafEth/derivatives/SfrxEth.sol

51:     function setMaxSlippage(uint256 _slippage) external onlyOwner {

60:     function withdraw(uint256 _amount) external onlyOwner {

File: contracts/SafEth//SafEth.sol

138:     function rebalanceToWeights() external onlyOwner {

165:     function adjustWeight(

182:     function addDerivative(

202:     function setMaxSlippage(

214:     function setMinAmount(uint256 _minAmount) external onlyOwner {

223:     function setMaxAmount(uint256 _maxAmount) external onlyOwner {

232:     function setPauseStaking(bool _pause) external onlyOwner {

241:     function setPauseUnstaking(bool _pause) external onlyOwner {

 

[G-02] Use assembly to calculate hashes

Saves 5000 deployment gas per instance and 80 runtime gas per instance.

Unoptimized

function solidityHash(uint256 a, uint256 b) public view {
	//unoptimized
	keccak256(abi.encodePacked(a, b));
}

Optimized

function assemblyHash(uint256 a, uint256 b) public view {
	//optimized
	assembly {
		mstore(0x00, a)
		mstore(0x20, b)
		let hashedVal := keccak256(0x00, 0x40)
	}
}

Instances: 6

File: contracts/SafEth/derivatives/Reth.sol

69:                 keccak256(

124:                 keccak256(

135:                 keccak256(

161:                 keccak256(

190:                     keccak256(

232:                 keccak256(

 

[G-03] Do not compare boolean expressions to boolean literals

<x> == true <=> <x>, also <x> == false <=> !<x>

Instances: 2

File: contracts/SafEth//SafEth.sol

64:         require(pauseStaking == false, "staking is paused");

109:         require(pauseUnstaking == false, "unstaking is paused");

 

[G-04] Division/Multiplication by 2 should use bit shifting

<x> * 2 is equivalent to <x> << 1 and <x> / 2 is the same as <x> >> 1. The MUL and DIV opcodes cost 5 gas, whereas SHL and SHR only cost 3 gas.

Instances: 1

File: contracts/SafEth/derivatives/Reth.sol

241:         return (sqrtPriceX96 * (uint(sqrtPriceX96)) * (1e18)) >> (96 * 2);

 

[G-05] Use indexed to save gas

Using indexed for value type event parameters (address, uint, bool) saves at least 80 gas in emitting the event (https://gist.github.com/Tomosuke0930/9bf61e01a8c3e214d95a9b84dcb41d97).

Note that for other types however (string, bytes), it is more expensive.

Instances: 6

File: contracts/SafEth//SafEth.sol

25:     event SetMaxSlippage(uint256 indexed index, uint256 slippage);

26:     event Staked(address indexed recipient, uint ethIn, uint safEthOut);

27:     event Unstaked(address indexed recipient, uint ethOut, uint safEthIn);

28:     event WeightChange(uint indexed index, uint weight);

31:         uint weight,

32:         uint index

 

[G-06] Use unchecked for operations that cannot overflow/underflow

By bypassing Solidity's built in overflow/underflow checks using unchecked, we can save gas. This is especially beneficial for the index variable within for loops (saves 120 gas per iteration).

Instances: 7

File: contracts/SafEth//SafEth.sol

71:         for (uint i = 0; i < derivativeCount; i++)

84:         for (uint i = 0; i < derivativeCount; i++) {

113:         for (uint256 i = 0; i < derivativeCount; i++) {

140:         for (uint i = 0; i < derivativeCount; i++) {

147:         for (uint i = 0; i < derivativeCount; i++) {

171:         for (uint256 i = 0; i < derivativeCount; i++)

191:         for (uint256 i = 0; i < derivativeCount; i++)

 

[G-07] Use private rather than public for constants

Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table. If needed to be viewed externally, the values can be read from the verified contract source code.

Instances: 11

File: contracts/SafEth/derivatives/Reth.sol

20:     address public constant ROCKET_STORAGE_ADDRESS =

22:     address public constant W_ETH_ADDRESS =

24:     address public constant UNISWAP_ROUTER =

26:     address public constant UNI_V3_FACTORY =

File: contracts/SafEth/derivatives/WstEth.sol

13:     address public constant WST_ETH =

15:     address public constant LIDO_CRV_POOL =

17:     address public constant STETH_TOKEN =

File: contracts/SafEth/derivatives/SfrxEth.sol

14:     address public constant SFRX_ETH_ADDRESS =

16:     address public constant FRX_ETH_ADDRESS =

18:     address public constant FRX_ETH_CRV_POOL_ADDRESS =

20:     address public constant FRX_ETH_MINTER_ADDRESS =

 

[G-08] Change public functions to external

Functions marked as public that are not called internally should be set to external to save gas and improve code quality. External call cost is less expensive than of public functions.

Instances: 8

File: contracts/SafEth/derivatives/Reth.sol

50:     function name() public pure returns (string memory) {

211:     function ethPerDerivative(uint256 _amount) public view returns (uint256) {

221:     function balance() public view returns (uint256) {

File: contracts/SafEth/derivatives/WstEth.sol

41:     function name() public pure returns (string memory) {

86:     function ethPerDerivative(uint256 _amount) public view returns (uint256) {

93:     function balance() public view returns (uint256) {

File: contracts/SafEth/derivatives/SfrxEth.sol

44:     function name() public pure returns (string memory) {

122:     function balance() public view returns (uint256) {

 

[G-09] Use named return values

Using named return values instead of explicitly calling return saves gas.

Instances: 15

File: contracts/SafEth/derivatives/Reth.sol

50:     function name() public pure returns (string memory) {

66:     function rethAddress() private view returns (address) {

120:     function poolCanDeposit(uint256 _amount) private view returns (bool) {

156:     function deposit() external payable onlyOwner returns (uint256) {

211:     function ethPerDerivative(uint256 _amount) public view returns (uint256) {

221:     function balance() public view returns (uint256) {

228:     function poolPrice() private view returns (uint256) {

File: contracts/SafEth/derivatives/WstEth.sol

41:     function name() public pure returns (string memory) {

73:     function deposit() external payable onlyOwner returns (uint256) {

86:     function ethPerDerivative(uint256 _amount) public view returns (uint256) {

93:     function balance() public view returns (uint256) {

File: contracts/SafEth/derivatives/SfrxEth.sol

44:     function name() public pure returns (string memory) {

94:     function deposit() external payable onlyOwner returns (uint256) {

111:     function ethPerDerivative(uint256 _amount) public view returns (uint256) {

122:     function balance() public view returns (uint256) {

 

#0 - c4-sponsor

2023-04-10T17:53:47Z

toshiSat marked the issue as sponsor confirmed

#1 - c4-judge

2023-04-23T15:35:18Z

Picodes marked the issue as grade-b

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter