search

code-423n4 / 2023-07-amphora-findings

3 stars 2 forks source link

Risk of Loss of Funds due to Improper Handling of BaseRewardPool Update #388

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-amphora/blob/daae020331404647c661ab534d20093c875483e1/core/solidity/contracts/core/VaultController.sol#L395-L401

Vulnerability details

In VaultController.sol#updateRegisteredErc20() when the owner updates the poolId, it may inadvertently change the address of the Convex BaseRewardPool associated with the token, which can impact the rewards functionality and result in potential losses for users.

The code responsible for handling the update is as follows:

function updateRegisteredErc20(
    address _tokenAddress,
    uint256 _ltv,
    address _oracleAddress,
    uint256 _liquidationIncentive,
    uint256 _cap,
    uint256 _poolId
) external override onlyOwner {
    // ... (omitted for brevity)

    if (_poolId != 0) {
        (address _lpToken,,, address _crvRewards,,) = BOOSTER.poolInfo(_poolId);
        if (_lpToken != _tokenAddress) revert VaultController_TokenAddressDoesNotMatchLpAddress();
        _collateral.collateralType = CollateralType.CurveLPStakedOnConvex;
        _collateral.crvRewardsContract = IBaseRewardPool(_crvRewards);
        _collateral.poolId = _poolId;
    }

    // ... (omitted for brevity)
}

The updateRegisteredErc20 function allows the owner of the VaultController.sol contract to update the poolId associated with an ERC20 token. This means that the contract owner can change the address of the BaseRewardPool (named _crvRewards in the code) associated with the token. If this update is performed without proper consideration, it could lead to a loss of funds for users who have deposited tokens into the vault and accumulated rewards in the previous BaseRewardPool.

The risk arises when the Convex protocol shuts down a pool associated with a specific poolId and creates a new pool with the same _tokenAddress. If the contract owner updates the poolId for the corresponding ERC20 token to point to the new pool without handling the transition of user rewards properly, users may lose access to their original rewards stored in the old BaseRewardPool.

Impact

Users may lose access to their original rewards.

Proof of Concept

consider the following scenario:

  1. A user deposits tokens of token (X) into the vault and starts accumulating rewards in the BaseRewardPool associated with poolId (A).

  2. The Convex protocol shuts down a pool with poolId (A), associated with an ERC20 token (X). After a while the Convex protocol create a new pool with poolId (B).

  3. The contract owner updates the poolId for token (X) from poolId (A) to poolId (B), pointing to a new pool created by Convex.

  4. The user tries to withdraw their deposited token amount and rewards from token (A) using the updated poolId (B).

  5. The withdrawal function interacts with the new BaseRewardPool associated with pool (B), causing the user to lose access to their original rewards stored in the old BaseRewardPool (pool (A)).

Tools Used

VSC

Recommended Mitigation Steps

Assessed type

Other

c4-pre-sort commented 1 year ago

minhquanym marked the issue as primary issue

c4-sponsor commented 1 year ago

0xShaito marked the issue as disagree with severity

0xShaito commented 1 year ago

Governance should not pass proposal that are harmful for the protocol and they should check the parameters carefully.

c4-judge commented 1 year ago

dmvt changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

dmvt marked the issue as grade-b

khalidfsh commented 1 year ago

Passing a proposal that addresses the issue, as well as not passing any proposal at all, could both have detrimental effects on the protocol. This is precisely why the update functionality exists.

There are notable concerns regarding the relationship between this issue and #90:

  1. Pool Updates and Shutdowns: Both vulnerabilities revolve around Convex pools linked to a specific poolId undergoing updates or shutdowns. These vulnerabilities emerge due to mishandling of these changes within the VaultController contract.

  2. Impacted Functionality: In both instances, users' ability to interact with the vault, deposit, and withdraw assets is affected. This issue pertains to rewards functionality, where users might lose access to their accumulated rewards if the poolId is updated without proper handling. The second issue impacts users' ability to deposit assets into the vault and could potentially lead to challenges with liquidations and collateral management.

  3. Risk to Users: Both vulnerabilities pose risks for users who have assets in the vault. Here, users could lose their rewards if pool updates are not managed appropriately. In issue 90, users could encounter difficulties depositing or withdrawing assets, with an added risk of improper liquidation.

  4. Mitigation Steps: Recommended mitigation steps for both vulnerabilities include implementing checks and appropriate handling of pool updates and shutdowns. In particular, both advocate for checks before updating or utilizing a poolId, along with effective coordination with the Convex protocol to manage transitions and updates.

In view of these aspects, I am of the opinion that this issue shares a partial relation with #90. I kindly request your consideration in acknowledging this interconnectedness.

Thank you @dmvt, for your valuable time that enlightening us with your considerations.

dmvt commented 1 year ago

Your issue is well written and related to the other in terms of the fix. The difference is that your report relies on governance making a mistake, whereas the other does not. The likelihood that governance would make this mistake is what pulls yours down to QA. The validity of the scenario you describe and its impact is why it was downgraded instead of invalidated.