reassor - Asymmetry contest

Findings Information

🌟 Selected for report: lukris02

Also found by: Bauer, HollaDieWaldfee, RedTiger, T1MOH, dec3ntraliz3d, joestakey, koxuan, qpzm, rbserver, reassor

Labels

bug

3 (High Risk)

satisfactory

duplicate-641

Awards

236.4864 USDC - $236.49

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L115-L116

Vulnerability details

Impact

Function SfrxEth.ethPerDerivative in order to calculate the value of Sfrx derivative in ETH first gets amount of frx that can be retrieved by using convertToAssets function and then executes following calculation:

return ((10 ** 18 * frxAmount) /
    IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle());

This is incorrect since price_oracle returns the value of the asset in ETH thus the correct calculation is:

(frxAmount * IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle()) / 1e18;

Proof of Concept

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L115-L116

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to fix the calculation:

(frxAmount * IFrxEthEthPool(FRX_ETH_CRV_POOL_ADDRESS).price_oracle()) / 1e18;

#0 - c4-pre-sort
2023-04-04T16:49:05Z

0xSorryNotSorry marked the issue as duplicate of #698

#1 - c4-judge
2023-04-21T16:04:53Z

Picodes marked the issue as satisfactory

#2 - c4-judge
2023-04-21T16:05:13Z

Picodes marked the issue as not a duplicate

#3 - c4-judge
2023-04-21T16:05:25Z

Picodes marked the issue as duplicate of #641

Findings Information

🌟 Selected for report: __141345__

Also found by: 0xWaitress, 0xbepresent, AkshaySrivastav, Bauer, Haipls, HollaDieWaldfee, Lirios, MiloTruck, SaeedAlipoor01988, UdarTeam, adriro, bytes032, ck, d3e4, hihen, hl_, kaden, ladboy233, lopotras, m_Rassska, peanuts, reassor, volodya

Awards

8.2654 USDC - $8.27

Labels

bug

2 (Med Risk)

satisfactory

duplicate-770

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L94-L106

Vulnerability details

Impact

Function SfrxEth.deposit uses external Frax contract frxETHMinter to deposit ether via submitAndDeposit.

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L70-L82

    function submitAndDeposit(address recipient) external payable returns (uint256 shares) {
        // Give the frxETH to this contract after it is generated
        _submit(address(this));

        // Approve frxETH to sfrxETH for staking
        frxETHToken.approve(address(sfrxETHToken), msg.value);

        // Deposit the frxETH and give the generated sfrxETH to the final recipient
        uint256 sfrxeth_recieved = sfrxETHToken.deposit(msg.value, recipient);
        require(sfrxeth_recieved > 0, 'No sfrxETH was returned');

        return sfrxeth_recieved;
    }

Execution starts with triggering internal function _submit that has a check if submitting is not paused:

https://github.com/code-423n4/2022-09-frax/blob/55ea6b1ef3857a277e2f47d42029bc0f3d6f9173/src/frxETHMinter.sol#L85-L101

    function _submit(address recipient) internal nonReentrant {
        // Initial pause and value checks
        require(!submitPaused, "Submit is paused");
        require(msg.value != 0, "Cannot submit 0");

        // Give the sender frxETH
        frxETHToken.minter_mint(recipient, msg.value);

        // Track the amount of ETH that we are keeping
        uint256 withheld_amt = 0;
        if (withholdRatio != 0) {
            withheld_amt = (msg.value * withholdRatio) / RATIO_PRECISION;
            currentWithheldETH += withheld_amt;
        }

        emit ETHSubmitted(msg.sender, recipient, msg.value, withheld_amt);
    }

The issue is that depositing will fail in case Frax's submitting is paused which leads to denial of service vulnerability.

Proof of Concept

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to check if submitting is paused via frxETHMinter.submitPaused() in SfrxEth.deposit function. In addition consider using Curve's pool to get Frax if direct deposit is not possible (in a similar way as for Rocket Pool).

#0 - c4-pre-sort
2023-04-04T22:01:42Z

0xSorryNotSorry marked the issue as duplicate of #328

#1 - c4-judge
2023-04-21T10:47:07Z

Picodes marked the issue as duplicate of #770

#2 - c4-judge
2023-04-24T18:30:20Z

Picodes marked the issue as satisfactory

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

42.0604 USDC - $42.06

Labels

bug

grade-a

QA (Quality Assurance)

sponsor acknowledged

Q-79

External Links

Link to GitHub issue

List

Low Risk

Incorrect private functions naming
Missing validation in setMaxSlippage
Critical address change
Missing validation in setMinAmount and setMaxAmount
Changing derivatives number
Derivatives contracts are missing events

Non-Critical Risk

Confusing naming poolPrice and canPoolDeposit
Missing natspec
The contracts use unlocked pragma
Use scientific notation for big numbers
Usage of magic numbers
Short derivatives names
Protocol only supports derivatives with 18 decimals

1. Incorrect private functions naming

Risk

Low

Impact

Contract Reth implements multiple private functions that do not follow correct naming convention. This makes code less readable and more prone to errors. Private function names should start with underscore _.

Proof of Concept

Reth.sol:

Recommended Mitigation Steps

It is recommended to start naming of all private functions with underscore _.

2. Missing validation in setMaxSlippage

Risk

Low

Impact

Function SafEth.setMaxSlippage is missing validation of _slippage parameter which might be accidentally set to incorrect value.

Proof of Concept

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L202-L208

Recommended Mitigation Steps

It is recommended to add validation to parameter _slippage to ensure that its value is between well defined range.

3. Critical address change

Risk

Low

Impact

Changing critical addresses such as ownership should be a two-step process where the first transaction (from the old/current address) registers the new address (i.e. grants ownership) and the second transaction (from the new address) replaces the old address with the new one. This gives an opportunity to recover from incorrect addresses mistakenly used in the first step. If not, contract functionality might become inaccessible.

Proof of Concept

SafEth.sol:

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L18

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to implement two-step process for changing ownership.

4. Missing validation in setMinAmount and setMaxAmount

Risk

Low

Impact

Functions SafEth.setMinAmount and SafEth.setMaxAmount are missing validation for the _minAmount and _maxAmount parameters which might be accidentally set to incorrect values.

Proof of Concept

Recommended Mitigation Steps

It is recommended to validate parameters _minAmount and _maxAmount to make sure that their values are in well defined ranges.

5. Denial of service - for loops over derivatives

Risk

Low

Impact

The protocol works on derivatives that mapping index keep increasing on adding new derivative. This might lead to denial of service conditions since protocol in functions such as stake or unstake iterates over all derivatives, also over these that were removed and their weight was set to 0.

Proof of Concept

https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L113-L119

Recommended Mitigation Steps

Consider redesigning protocol in a away it will be not iterating over large number of derivatives.

6. Derivatives contracts are missing events

Risk

Low

Impact

All derivatives contracts are not implementing events for critical functions. Lack of events emission makes it difficult for off-chain applications to monitor the protocol.

Proof of Concept

Reth.sol:

SfrxEth.sol:

WstEth.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing events to listed functions.

7. Confusing naming poolPrice and canPoolDeposit

Risk

Non-Critical

Impact

Contract Reth implements functions poolPrice and poolCanDeposit that have confusing names which makes code less readable and might lead to errors. Function poolPrice refers to the price from the uniswap pool, while poolCanDeposit checks if its possible to deposit to the Rocket Pool.

Proof of Concept

Recommended Mitigation Steps

It is recommended to change the name of the functions so they reflect which pools they refer to.

8. Missing natspec

Risk

Non-Critical

Impact

Multiple contracts are missing natspec comments which makes code more difficult to read and prone to errors.

Proof of Concept

SafEth.sol

Incorrect natspec - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L157-L164

Reth.sol:

Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L47-L50
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L62-L66
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L75-L83
Missing @param amount param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L104-L107
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L116-L120
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L152-L156
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L206-L211
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L218-L221
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L225-L228

SfrxEth.sol:

Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L41-L44
Missing @param _slippage param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L48-L51
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L90-L94
Missing @param _amount and @return params - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L108-L111
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L119-L122

WstEth.sol:

Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L38-L41
Missing @param _slippage param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L45-L48
Missing @param _amount param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L52-L56
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L69-L73
Missing @param _amount and @return params - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L83-L86
Missing @return param - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L90-L93

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to add missing natspec comments.

9. The contracts use unlocked pragma

Risk

Non-Critical

Impact

As different compiler versions have critical behavior specifics if the contract gets accidentally deployed using another compiler version compared to one they tested with, various types of undesired behavior can be introduced.

Proof of Concept

SafEth.sol - ^0.8.13 - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L2
Reth.sol - ^0.8.13 - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L2
SfrxEth.sol - ^0.8.13 - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L2
WstEth.sol - ^0.8.13 - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L2

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

Consider locking compiler version, for example pragma solidity 0.8.17.

10. Use scientific notation for big numbers

Risk

Non-Critical

Impact

All contracts are using math calculations to express big numbers instead of using scientific notation.

Proof of Concept

SafEth.sol:

Reth.sol:

SfrxEth.sol:

WstEth.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

It is recommended to use scientific notation, for example: 1e18.

11. Usage of magic numbers

Risk

Non-Critical

Impact

The protocol is using a lot of "magic numbers" which makes code less readable and more prone to errors.

Proof of Concept

SafEth.sol:

Use 0.5 ether and 200 ether - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/SafEth.sol#L54-L55

Reth.sol:

Use POOL_FEE constant - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L180
Use POOL_FEE constant - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/Reth.sol#L238

SfrxEth.sol:

Use constant values ETH=0 and FRX=1 for exchange - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/SfrxEth.sol#L77-L82
Use constant values ETH=0 and STETH=1 for exchange - https://github.com/code-423n4/2023-03-asymmetry/blob/44b5cd94ebedc187a08884a7f685e950e987261c/contracts/SafEth/derivatives/WstEth.sol#L61

Recommended Mitigation Steps

Consider adding constant variables with self-explanatory names.

12. Short derivatives names

Risk

Non-Critical

Impact

The protocol is using short names for derivatives such as "Lido" or "Frax" which might lead to confusion.

Proof of Concept

Recommended Mitigation Steps

Consider adding more detailed names for derivatives.

13. Protocol supports only derivatives with 18 decimals

Risk

Non-Critical

Impact

The protocol supports only derivatives that use 18 decimals. This assumption is carried in all calculation performed by the protocol.

Proof of Concept

SafEth.sol:

Tools Used

Manual Review / VSCode

Recommended Mitigation Steps

Consider adding support for derivatives with different number of decimals.

#0 - c4-sponsor
2023-04-10T19:42:52Z

toshiSat marked the issue as sponsor acknowledged

#1 - c4-judge
2023-04-24T18:34:59Z

Picodes marked the issue as grade-a

Asymmetry contest - reassor's results

A protocol to help diversify and decentralize liquid staking derivatives.

General Information

Findings Distribution

Researcher Performance

External Links

[H-04] Incorrect calculation of SfrxETH derivative value - $236.49

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - c4-pre-sort2023-04-04T16:49:05Z

#1 - c4-judge2023-04-21T16:04:53Z

#2 - c4-judge2023-04-21T16:05:13Z

#3 - c4-judge2023-04-21T16:05:25Z

[M-06] Missing check in SfrxEth if direct depositing in Frax is possible - $8.27

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - c4-pre-sort2023-04-04T22:01:42Z

#1 - c4-judge2023-04-21T10:47:07Z

#2 - c4-judge2023-04-24T18:30:20Z

[Q-79] QA Report - $42.06

Findings Information

Awards

Labels

External Links

List

Low Risk

Non-Critical Risk

1. Incorrect private functions naming

Risk

Impact

Proof of Concept

Recommended Mitigation Steps

2. Missing validation in setMaxSlippage

Risk

Impact

Proof of Concept

Recommended Mitigation Steps

3. Critical address change

Risk

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

4. Missing validation in setMinAmount and setMaxAmount

Risk

Impact

Proof of Concept

Recommended Mitigation Steps

5. Denial of service - for loops over derivatives

Risk

Impact

Proof of Concept

Recommended Mitigation Steps

6. Derivatives contracts are missing events

Risk

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

7. Confusing naming poolPrice and canPoolDeposit

Risk

Impact

Proof of Concept

Recommended Mitigation Steps

#0 - c4-pre-sort
2023-04-04T16:49:05Z

#1 - c4-judge
2023-04-21T16:04:53Z

#2 - c4-judge
2023-04-21T16:05:13Z

#3 - c4-judge
2023-04-21T16:05:25Z

#0 - c4-pre-sort
2023-04-04T22:01:42Z

#1 - c4-judge
2023-04-21T10:47:07Z

#2 - c4-judge
2023-04-24T18:30:20Z

#0 - c4-sponsor
2023-04-10T19:42:52Z

#1 - c4-judge
2023-04-24T18:34:59Z