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

PirexGmx contract migration can be blocked #198

Closed
code423n4 opened this issue Nov 27, 2022 · 3 comments
Closed

PirexGmx contract migration can be blocked #198

code423n4 opened this issue Nov 27, 2022 · 3 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-61 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@code423n4
Copy link
Contributor

code423n4 commented Nov 27, 2022

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L930

Vulnerability details

Impact

For migration to work correctly the contract PirexGmx is using functions from IRewardRouterV2 contract.

It has to follow a sequence of first calling setPauseState(true) then initiateMigration and the final step is for the new contract to call completeMigration.

This process can be broken by sending at least one token of (Vested GMX) vGMX or (Vested GLP) vGLP to the PirexGMX contract because:

When calling initiateMigration the old contract is signaling its intent via gmxRewardRouterV2.signalTransfer(newContract); and by doing this the called contract is checking if we have any mentioned token in the contract balance. If we do then the call is reverted and the migration is stopped.

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L930

The migration is stopped because in the next sequence call the router (gmxRewardRouterV2) is reverting because we have failed to set the pendingReceivers (field is inside gmxRewardRouterV2 code) to our new contract address using our initiateMigration call.

// inside RewardRouterV2::acceptTransfer
require(pendingReceivers[_sender] == receiver, "RewardRouter: transfer not signalled");

The PirexGMX contract does not have any capacity to move this offending tokens out before initiating the migration process.

Proof of Concept

On both Arbitrum and Avalanche the suggested contract addresses for RewardRouterV2::signalTransfer this check is made like this:

function signalTransfer(address _receiver) external nonReentrant {
        require(IERC20(gmxVester).balanceOf(msg.sender) == 0, "RewardRouter: sender has vested tokens");
        require(IERC20(glpVester).balanceOf(msg.sender) == 0, "RewardRouter: sender has vested tokens");

        _validateReceiver(_receiver);
        pendingReceivers[msg.sender] = _receiver;
    }

Tools Used

Manual review

Recommended Mitigation Steps

And function to the PirexGMX contract owner to clear/move out the offending tokens before initiating the migration sequnence.

@code423n4 code423n4 added 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 labels Nov 27, 2022
code423n4 added a commit that referenced this issue Nov 27, 2022
@c4-judge
Copy link
Contributor

c4-judge commented Dec 4, 2022

Picodes marked the issue as primary issue

@c4-judge c4-judge added the primary issue Highest quality submission among a set of duplicates label Dec 4, 2022
@c4-judge
Copy link
Contributor

Picodes marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Dec 30, 2022
@C4-Staff
Copy link
Contributor

JeeberC4 marked the issue as duplicate of #61

@C4-Staff C4-Staff added duplicate-61 and removed primary issue Highest quality submission among a set of duplicates labels Jan 10, 2023
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-61 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

3 participants