Submitted by unforgiven, also found by eierina, Jeiwan, and imare
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L269-L293
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L346-L355
Function configureGmxState() of PirexGmx is for configuring GMX contract state but logic is using safeApprove() improperly, it wont reset approval amount for old stakedGmx address This would cause 4 problem:
This is configureGmxState() code in PirexGmx:
As you can see it just sets the approval for new stakeGmx address and doesnt do anything about spending approval of old stakeGmx address.
This is part of the setContract() code that handels stakeGmx:
As you can see it resets the spending approval of old stakedGmx address to zero and then give unlimited spending approval for new address.
So the impact #1 is obvious that the code for same logic in two different functions dont behave similarly.
Function configureGmxState() is used for configuring GMX contract state but if owner uses this function one time then it wont be possible to call this function the second time if stakedGmx wasnt changed. For example in this scenario:
So the impact #2 would happen and calls to this function in some scenarios would fail and it wont be functional.
Because function configureGmxState() doesnt reset the old stakeGmx addresses GMX token spending approval to 0x0 so it would be possible to lose all GMX balance of PirexGmx contract if the old stakeGmx addresses are compromised. For example in this scenario:
BPirexGmx wont set approval for old stakeGmx contract so it would be possible for that old stakeGmx address to transfer GMX balance of PirexGmx anytime in future. The bug in old stakeGmx contract or leakage of private keys of stakeGmx address (private key who can become the owner or admin of that contract) can happen after migrating GMX and Pirex contracts too. This is impact #3 scenario.
In scenario #4, contract would be stuck in unrecoverable state. The problem is that if configureGmxState() gets used more than once and stakeGmx variables value has been changes more than once then it wont be possible to set stakeGmx value to old values either with configureGmxState() or setContract() and the contract wont be useful. The scenario is this: (safeApprove() wont allow to set non-zero approval for address that has already non-zero approval amount. See the OZ implementation)
So in this state owner cant recover PirexGmx contract and because contract has wrong value for stakeGmx it wont be functional and it would stay in broken state.
VIM
Like setContract(), function configureGmxState() should set approval for old PirexGmx to zero first.
drahrealm (Redacted Cartel) confirmed, but disagreed with severity and commented:
Picodes (judge) commented:
