Frax Ether Liquid Staking contest - seyni's results

A liquid ETH staking derivative designed to uniquely leverage the Frax Finance ecosystem.

General Information

Platform: Code4rena

Start Date: 22/09/2022

Pot Size: $30,000 USDC

Total HM: 12

Participants: 133

Period: 3 days

Judge: 0xean

Total Solo HM: 2

Id: 165

League: ETH

Frax Finance

Findings Distribution

Researcher Performance

Rank: 50/133

Findings: 3

Award: $53.31

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Frax Finance

Findings Information

🌟 Selected for report: Lambda

Also found by: 0x1f8b, 0x5rings, 0xSky, 0xSmartContract, 8olidity, Chom, CodingNameKiki, IllIllI, Ruhum, Sm4rty, brgltd, hansfriese, m9800, magu, pashov, pedroais, peritoflores, prasantgupta52, rokinot, seyni

Awards

12.4859 USDC - $12.49

Labels

bug

duplicate

2 (Med Risk)

External Links

Link to GitHub issue

Lines of code

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

Vulnerability details

Impact

Some tokens, like USDT, does not return a boolean value indicating success or failure when using transfer/transferFrom, instead their functions transfer/transferFrom return void. If such tokens are sent to the contract they will not be recoverable using frxETHMinter.recoverERC20.

The same issue has been found in a previous contest :

https://github.com/code-423n4/2022-05-runes-findings/issues/70

https://github.com/code-423n4/2022-05-runes-findings/issues/63

Proof of Concept

The tokenAddress is cast to type IERC20 which expects a bool to be returned calling transfer, but for tokens that does not return a boolean for transfer (like USDT) the function call will always revert even if the token transfer should have been successful.

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

    function recoverERC20(address tokenAddress, uint256 tokenAmount) external onlyByOwnGov {
        require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");


        emit EmergencyERC20Recovered(tokenAddress, tokenAmount);
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Consider using SafeERC20 or SafeTransferLib which both accepts ERC20 token with no boolean return value like USDT.

#0 - FortisFortuna
2022-09-25T21:30:46Z

Not really medium risk. Technically you could use safeTransfer, but if someone were to accidentally send something to this contract, it would most likely be either ETH, FRAX, frxETH, or sfrxETH, all of which are transfer compliant.

#1 - joestakey
2022-09-26T15:24:10Z

Duplicate of #18

Findings Information

🌟 Selected for report: rotcivegaf

Also found by: 0x040, 0x1f8b, 0x4non, 0xNazgul, 0xSmartContract, 0xf15ers, 8olidity, Aymen0909, B2, Bahurum, Bnke0x0, Ch_301, CodingNameKiki, Deivitto, Diana, Funen, IllIllI, JC, JLevick, KIntern_NA, Lambda, OptimismSec, PaludoX0, RockingMiles, Rolezn, Sm4rty, Soosh, Tagir2003, Tointer, TomJ, Triangle, Trust, V_B, Waze, Yiko, __141345__, a12jmx, ajtra, asutorufos, ayeslick, aysha, bbuddha, bharg4v, bobirichman, brgltd, bytera, c3phas, cryptostellar5, cryptphi, csanuragjain, datapunk, delfin454000, durianSausage, exd0tpy, gogo, got_targ, jag, joestakey, karanctf, ladboy233, leosathya, lukris02, mics, millersplanet, natzuu, neko_nyaa, obront, oyc_109, parashar, peritoflores, rbserver, ret2basic, rokinot, ronnyx2017, rvierdiiev, sach1r0, seyni, sikorico, slowmoses, tnevler, yasir, yongskiws

Awards

28.0141 USDC - $28.01

Labels

bug

QA (Quality Assurance)

External Links

Link to GitHub issue

QA

Low Severity

[L-01] Typo in variable

The variable is called sfrxeth_recieved instead of sfrxeth_received.

Files links :

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

Findings :

2022-09-frax/src/frxETHMinter.sol:78:        uint256 sfrxeth_recieved = sfrxETHToken.deposit(msg.value, recipient);
2022-09-frax/src/frxETHMinter.sol:79:        require(sfrxeth_recieved > 0, 'No sfrxETH was returned');
2022-09-frax/src/frxETHMinter.sol:81:        return sfrxeth_recieved;

Non-critical

[N-01] Floating pragma compiler version

Contracts should not use a floating pragma in order to ensure that the code has been tested and deployed with the same version.

Files links :

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol

https://github.com/code-423n4/2022-09-frax/blob/main/src/sfrxETH.sol

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol

Findings :

2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:2: 	 pragma solidity ^0.8.0;
2022-09-frax/src/OperatorRegistry.sol:2: 	 pragma solidity ^0.8.0;
2022-09-frax/src/frxETHMinter.sol:2: 	 pragma solidity ^0.8.0;
2022-09-frax/src/sfrxETH.sol:2: 	 pragma solidity ^0.8.0;

Findings Information

🌟 Selected for report: pfapostol

Also found by: 0x040, 0x1f8b, 0x4non, 0x5rings, 0xA5DF, 0xNazgul, 0xSmartContract, 0xmatt, 0xsam, Amithuddar, Aymen0909, B2, Ben, Bnke0x0, Chom, CodingNameKiki, Deivitto, Diana, Fitraldys, Funen, IllIllI, JAGADESH, JC, Metatron, Ocean_Sky, PaludoX0, Pheonix, RaymondFam, ReyAdmirado, RockingMiles, Rohan16, Rolezn, Satyam_Sharma, Sm4rty, SnowMan, SooYa, Tagir2003, TomJ, Tomio, Triangle, V_B, Waze, __141345__, ajtra, albincsergo, asutorufos, aysha, beardofginger, bobirichman, brgltd, bulej93, bytera, c3phas, ch0bu, cryptostellar5, cryptphi, d3e4, delfin454000, dharma09, drdr, durianSausage, emrekocak, erictee, fatherOfBlocks, gogo, got_targ, imare, jag, karanctf, ladboy233, leosathya, lukris02, medikko, mics, millersplanet, natzuu, neko_nyaa, oyc_109, peanuts, prasantgupta52, rbserver, ret2basic, rokinot, ronnyx2017, rotcivegaf, sach1r0, samruna, seyni, slowmoses, tnevler, wagmi, zishansami

Awards

12.8108 USDC - $12.81

Labels

bug

G (Gas Optimization)

External Links

Link to GitHub issue

Gas optimizations

[G-01] Useless explicit initialization of variable to default value

Explicitly initializing a variable with its default value wastes gas.

Files links :

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol

Findings :

2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:84: 	 for (uint i = 0; i < minters_array.length; i++){
2022-09-frax/src/OperatorRegistry.sol:63: 	 for (uint256 i = 0; i < arrayLength; ++i) {
2022-09-frax/src/OperatorRegistry.sol:84: 	 for (uint256 i = 0; i < times; ++i) {
2022-09-frax/src/OperatorRegistry.sol:114: 	 for (uint256 i = 0; i < original_validators.length; ++i) {
2022-09-frax/src/frxETHMinter.sol:94: 	 uint256 withheld_amt = 0;
2022-09-frax/src/frxETHMinter.sol:129: 	 for (uint256 i = 0; i < numDeposits; ++i) {

[G-02] Cache Array Length Outside of Loop

Reading array length at each iteration of a loop uses 6 gas per loop (3 for mload and 3 to place memory_offset in the stack).

Files links :

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol

Findings :

2022-09-frax/src/OperatorRegistry.sol:114: 	 for (uint256 i = 0; i < original_validators.length; ++i) {

[G-03] `i++` instead of `++i`

i++ costs 5 more gas per loop than ++i.

Files links :

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol

Findings :

2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:84:   for (uint i = 0; i < minters_array.length; i++){

[G-05] Inline `unchecked{++i;}` could be used

The increment of a loop can be inlined and unchecked since there is no risk of overflow/underflow, which cost less gas.

Files links :

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol

Findings :

2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:84: 	 for (uint i = 0; i < minters_array.length; i++){
2022-09-frax/src/frxETHMinter.sol:129:   for (uint256 i = 0; i < numDeposits; ++i) {
2022-09-frax/src/OperatorRegistry.sol:63:   for (uint256 i = 0; i < arrayLength; ++i) {
2022-09-frax/src/OperatorRegistry.sol:84:   for (uint256 i = 0; i < times; ++i) {
2022-09-frax/src/OperatorRegistry.sol:114:   for (uint256 i = 0; i < original_validators.length; ++i) {

[G-06] Boolean comparison

Using if(bool) or if(!bool) instead of if(bool == true) or if(bool == false) costs less gas.

Files links :

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol

Findings :

2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:46: 	 require(minters[msg.sender] == true, "Only minters");
2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:68: 	 require(minters[minter_address] == false, "Address already exists");
2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:78: 	 require(minters[minter_address] == true, "Address nonexistant");

[G-7] Use custom errors instead of revert string

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met).

Files links :

https://github.com/code-423n4/2022-09-frax/blob/main/src/ERC20/ERC20PermitPermissionedMint.sol

https://github.com/code-423n4/2022-09-frax/blob/main/src/OperatorRegistry.sol

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol

Findings :

2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:41: 	 require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");
2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:46: 	 require(minters[msg.sender] == true, "Only minters");
2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:66: 	 require(minter_address != address(0), "Zero address detected");
2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:68: 	 require(minters[minter_address] == false, "Address already exists");
2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:77: 	 require(minter_address != address(0), "Zero address detected");
2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:78: 	 require(minters[minter_address] == true, "Address nonexistant");
2022-09-frax/src/ERC20/ERC20PermitPermissionedMint.sol:95: 	 require(_timelock_address != address(0), "Zero address detected");
2022-09-frax/src/OperatorRegistry.sol:46: 	 require(msg.sender == timelock_address || msg.sender == owner, "Not owner or timelock");
2022-09-frax/src/OperatorRegistry.sol:137: 	 require(numVals != 0, "Validator stack is empty");
2022-09-frax/src/OperatorRegistry.sol:182: 	 require(numValidators() == 0, "Clear validator array first");
2022-09-frax/src/OperatorRegistry.sol:203: 	 require(_timelock_address != address(0), "Zero address detected");
2022-09-frax/src/frxETHMinter.sol:79: 	 require(sfrxeth_recieved > 0, 'No sfrxETH was returned');
2022-09-frax/src/frxETHMinter.sol:87: 	 require(!submitPaused, "Submit is paused");
2022-09-frax/src/frxETHMinter.sol:88: 	 require(msg.value != 0, "Cannot submit 0");
2022-09-frax/src/frxETHMinter.sol:122: 	 require(!depositEtherPaused, "Depositing ETH is paused");
2022-09-frax/src/frxETHMinter.sol:126: 	 require(numDeposits > 0, "Not enough ETH in contract");
2022-09-frax/src/frxETHMinter.sol:140: 	 require(!activeValidators[pubKey], "Validator already has 32 ETH");
2022-09-frax/src/frxETHMinter.sol:160: 	 require (newRatio <= RATIO_PRECISION, "Ratio cannot surpass 100%");
2022-09-frax/src/frxETHMinter.sol:167: 	 require(amount <= currentWithheldETH, "Not enough withheld ETH in contract");
2022-09-frax/src/frxETHMinter.sol:171: 	 require(success, "Invalid transfer");
2022-09-frax/src/frxETHMinter.sol:193: 	 require(success, "Invalid transfer");
2022-09-frax/src/frxETHMinter.sol:200: 	 require(IERC20(tokenAddress).transfer(owner, tokenAmount), "recoverERC20: Transfer failed");

Frax Ether Liquid Staking contest - seyni's results

A liquid ETH staking derivative designed to uniquely leverage the Frax Finance ecosystem.

General Information

Findings Distribution

Researcher Performance

External Links

[M-06] IERC20.transfer does not support non-standard ERC20 tokens - $12.49

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

Tools Used

Recommended Mitigation Steps

#0 - FortisFortuna2022-09-25T21:30:46Z

#1 - joestakey2022-09-26T15:24:10Z

[Q-31] QA Report - $28.01

Findings Information

Awards

Labels

External Links

QA

Low Severity

[L-01] Typo in variable

Files links :

Findings :

Non-critical

[N-01] Floating pragma compiler version

Files links :

Findings :

[G-18] Gas Report - $12.81

Findings Information

Awards

Labels

External Links

Gas optimizations

[G-01] Useless explicit initialization of variable to default value

Files links :

Findings :

[G-02] Cache Array Length Outside of Loop

Files links :

Findings :

[G-03] i++ instead of ++i

Files links :

Findings :

[G-05] Inline unchecked{++i;} could be used

Files links :

Findings :

[G-06] Boolean comparison

Files links :

Findings :

[G-7] Use custom errors instead of revert string

Files links :

Findings :

#0 - FortisFortuna
2022-09-25T21:30:46Z

#1 - joestakey
2022-09-26T15:24:10Z

[G-03] `i++` instead of `++i`

[G-05] Inline `unchecked{++i;}` could be used