hats-finance / Convergence---Convex-integration-0xb3df23e155b74ad2b93777f58980d6727e8b40bb

0 stars 1 forks source link

`CvxConvergenceLocker::sentTokens` doesn't protect from newly added reward tokens #35

Open hats-bug-reporter[bot] opened 6 months ago

hats-bug-reporter[bot] commented 6 months ago

Github username: @NicolaMirchev Twitter username: nmirchev8 Submission hash (on-chain): 0x87b9161cbee385de744dd7e4eae4373eefec858382dde8fc4ab5117587d9ea75 Severity: low

Description: Description\ Inside CvxConvergenceLocker there is restricted function sendTokens, which has modifier onlyOwner and it is used to transfer locked funds in the contract, which are different from the reward tokens as we can see the following lines:

     require(address(tokens[i]) != address(CVX), "CVX_CANNOT_BE_TRANSFERRED");
            require(address(tokens[i]) != address(CRV), "CRV_CANNOT_BE_TRANSFERRED");
            require(address(tokens[i]) != address(FXS), "FXS_CANNOT_BE_TRANSFERRED");
            require(address(tokens[i]) != address(this), "CVGCVX_CANNOT_BE_TRANSFERRED");

The problem is that new reward tokens may be added from CVX_LOCKER using addReward, but sendTokens doesn't check for such new reward tokens and so owner may tranfer them out of the locker contract.

Attack Scenario\

  1. Owner of CvxLocker calls addReward with a new reward token different from [CVX, CRV, FXS] (maybe CVX1)

  2. Some reward value is accumulated for CvxConvergenceLocker and when CvxConvergenceLocker::pullRewards, it is transferred to the locker contract, but CVX1 is still not added to rewardTokensConfiguration, so it is left in this contract

  3. Now owner calls sendTokens with the address of CVX1 and user rewards are sent outside of the contract. Attachments

  4. Proof of Concept (PoC) File

  5. Revised Code File (Optional) Inside sendTokens check dynamically wheter tokens[i] is not present in CvxLocker::rewardTokens()

PlamenTSV commented 6 months ago

I agree it seems like a missed inconsistency, will ask about this and get back.

0xMR0 commented 6 months ago

@PlamenTSV i think, it is not valid issue. this issue considers future scenario which is not in scope also sendToken can be called by owner only so centralize issue, at best informational issue

walk-on-me commented 6 months ago

This is a valid issue We should iterate through the rewardTokenConfig array instead of these hardcoded addresses

It's indeed a Low issue