it's not possible to correctly change and update PirexGmx address in AutoPxGlp and AutoPxGmx contracts because setPlatform() don't set spending approval for new address #287
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
downgraded by judge
Judge downgraded the risk level of this issue
duplicate-182
satisfactory
satisfies C4 submission criteria; eligible for awards
Lines of code
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L126-L136
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L148-L158
Vulnerability details
Impact
Function
setPlatform()
inAutoPxGlp
andAutoPxGmx
contracts update thePirexGmx
address but because it doesn't set token(gmxBaseReward
token forAutoPxGlp
andgmx
token forAutoPxGmx
) spending approval for new address it would causeAutoPxGlp
andAutoPxGmx
to be in a broken state which depositing interaction withPirexGmx
wouldn't be possible. This would causecompound()
function to revert and because all the logics callcompound()
so all the logics ofAutoPxGlp
andAutoPxGmx
to fail. so ifowner
callssetPlatform()
then all the funds in the contract would be inaccessible andowner
should callsetPlatform()
again and set the old address ofPirexGmx
and if this old address was broken then funds would be locked forever. so the impact is:PirexGmx
inAutoPxGlp
andAutoPxGmx
if addresses in the protocol has been updated.PirexGmx
was deployed in new address and the contract in the old address of was broken.Proof of Concept
This is the
setPlatform()
code inAutoPxGlp
andAutoPxGmx
contracts:As you can see it sets the new address but don't give spending approval for the new address and don't set the approval for the old address as zero.
This is the
constructors()
code inAutoPxGmx
:As you can see it sets the unlimited spending approval of token
gmx
for platform address and this approval is required for contract logics to work correctly and if it wasn't there contract can't work withPirexGmx
and functioncompound()
would fail and all other function would fail because they callcompound()
function.This is
compound()
code inAutoPxGmx
:As you can see it deposits
gmx
balance of contract inplatform
and if the spending approval was not their the code would revert.The situation for
AutoPxGlp
contract is similar, theconstructor()
give unlimited spending approval of tokengmxBaseReward
forplatform
(meaningPirexGmx
contract address) address which is necessary to interact withPirexGmx
and without itcompound()
wouldn't work and all the other function that callcompound()
(which is all the important functionality of contract) would fail too.As you saw Function
setPlatform()
doesn't set proper approvals(likeconstructor()
) for newplatform
address so if admin can't use this function to updatePirexGmx
addresses inAutoPxGlp
andAutoPxGmx
and if was required to updatePirexGmx
address(for example it was hacked or the contract was broken or contract has been updated and is in new addresses) then contractsAutoPxGlp
andAutoPxGmx
would be broken and even there is chance that old the funds would be lucked in the contract. imagine this scenario:PirexGmx
and team deploy newPirexGmx
with new address.(the oldPirexGmx
is broken because of a hack or bug or it's just removed by team because they updated it in new address).owner
callssetPlatform()
inAutoPxGlp
andAutoPxGmx
to updatePirexGmx
contract address in those contracts. and code would update the platform address but because of the bug don't give spending approval of necessary tokens for new address.withdraw()
,redeem()
,deposit()
,mint()
would be broken because they callcompound()
in their execution flow andcompound()
tries to deposit tokens inPirexGmx
but the spending approval is zero.AutoPxGlp
andAutoPxGmx
would be locked for ever because contracts can't work with newPirexGmx
address and the contract in old address is broken or removed.Tools Used
VIM
Recommended Mitigation Steps
in
setPlatform()
code should set approval for old address to zero and approval for new address to max.The text was updated successfully, but these errors were encountered: