`uncommit` sends tokens to the wrong user

[code423n4]

Handle

cmichel

Vulnerability details

The PoolCommitter._uncommit function calls the ILeveragedPool(leveragedPool).quoteTokenTransfer/mintTokens function with msg.sender. But in _uncommit's case that's the pool, not the commit owner, see onlyPool modifier on executeAllCommitments which calls _uncommit.

Impact

Users lose all tokens from their commitments as they are sent / minted to the pool instead.

Recommended Mitigation Steps

Instead of msg.sender, use _commit.owner:

// minting: return quote tokens to the commit owner
// @audit msg.sender is pool, should be _commit.owner
ILeveragedPool(leveragedPool).quoteTokenTransfer(msg.sender, _commit.amount);
// same with mint cases

[kumar-ish]

This is a valid issue. However, it requires there to be an underlying bug in the contracts which would make the executeCommitment call in executeAllCommitments revert (as _uncommit is only called in that case). If the warden can find a way for executeCommitment to revert, we would consider this to be an issue of this severity but otherwise we disagree with the severity as it requires/needs to be paired with another bug in the contracts.

It's also worth noting that governance can rescue any funds (rather, quote/collateral tokens) from the LeveragedPool contract. So if there were to be a case where there was a bug in the contracts that led to executeCommitment reverting and the users having their mints (quote tokens sent back to the LeveragedPool), governance could pause the contracts, drain out the equivalent worth and set up another contract where users can burn their tokens/claim them; if they burned the tokens which later uncommitted, then governance could send them an equal amount in collateral tokens. On the same note, if a critical vulnerability in executeCommitment were to be happening whereby commits were being uncommitted, then governance could also pause the contracts, rescue the funds and do some combination of the efforts above to ensure users get the funds back securely.

[GalloDaSballo]

Would setting up a commit such that this line will underflow https://github.com/tracer-protocol/perpetual-pools-contracts/blob/646360b0549962352fe0c3f5b214ff8b5f73ba51/contracts/implementation/PoolCommitter.sol#L305 , causing a revert, be a way to cause the function to call _uncommit ?

[GalloDaSballo]

Also, to clarify, you're saying you believe the code will never call _uncommit as it won't ever revert, right?

[GalloDaSballo]

At this time I believe that if a user mistakenly commits more than the value in shadowPools[_commitType] they can cause a silent revert which will trigger the bug

I think only their own funds are at risk, and either passing along the original committer or storing it in the commitData would allow to safely return them their funds.

With the information I have this issue sits between medium and high severity, high severity because user funds are at risk, medium because:

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

As of now I'll mark as medium and valid.

Will think it over the weekend

If the sponsor can let me know their take and reply to the questions above, that can help clarify the severity and validity

[kumar-ish]

Would setting up a commit such that this line will underflow https://github.com/tracer-protocol/perpetual-pools-contracts/blob/646360b0549962352fe0c3f5b214ff8b5f73ba51/contracts/implementation/PoolCommitter.sol#L305 , causing a revert, be a way to cause the function to call _uncommit ?

@GalloDaSballo No. shadowPools is a mapping of commit types to the sum of pool tokens to be burned (rather, to be executed because they've already been burned), or sum of collateral tokens to be used in minting that haven't been used for minting yet. executeCommitment can only be called on Commit's, which are in the commits mapping, which can only be added to via the commit function where users have to commit to putting up collateral/burning their pool tokens, which is the function that increments shadowPools. I realise that sounds a bit convoluted, but basically executeCommitment and its _commit parameter has a direct dependency on users committing via the commit function, which increments shadowPools by the value of their commit (which they can't game -- their collateral tokens get sent to the LeveragedPool contract instantly and their tokens get burned instantly and they don't have access to those funds anymore).

Also, to clarify, you're saying you believe the code will never call _uncommit as it won't ever revert, right?

Yes, that's right. It is there as a fail-safe (so that if there was some bug in a commit that stopped a commit in the queue from being executed, it wouldn't stop the markets). We are refactoring this code nonetheless though.

[GalloDaSballo]

Alright from this information I understand that the underflow idea can't happen (gas optimization there would be to use unsafe operations I guess)

I think given the system a refactoring to send the funds back is warranted

That said the fact that there seems to be no way to get a revert excludes the high severity.

That leaves us with the finding either being med or low risk

Low risk would be acceptable as the code doesn't work as it suggests (_uncommit is never executed, and if it did it wouldn't reimburse the user)

The alternative take is Medium: if _uncommit where executed it would cause in a loss of funds / funds stuck

As of now I'll leave it as med, while we don't have a way to trigger _uncommit we can still make the claim that if _uncommit where to run, it wouldn't reimburse the user

[GalloDaSballo]

Going a little deeper for the sake of clarity:

The math library is programmed to never revert: https://github.com/tracer-protocol/perpetual-pools-contracts/blob/646360b0549962352fe0c3f5b214ff8b5f73ba51/contracts/implementation/PoolSwapLibrary.sol#L256

The amounts in commit are always greater than 0 so no revert there

The pool.setter is innocuous

The only thing I found is the pool.quoteTokenTransfer(_commit.owner, amountOut);

If for an unfortunate reason the pool is drained from the quoteToken and the safeTransfer fail, then the function would revert

On that note the way to perform this would be to use function setKeeper(address _keeper) external override onlyGov onlyUnpaused { To change the keeper to a EOA / Malicious account

And then run function payKeeperFromBalances(address to, uint256 amount) with the full amount (or close to it) of amount <= shortBalance + longBalance

This seems to be a permissioned way (admin privilege) to rug funds from the LeveragedPool as well as enabling the _uncommit to be triggered

Given these findings (which I may misunderstand, so feel free to correct me) I highly recommend the sponsor to ensure there's a timelock for changing keeper Additionally (and I may be missing something) allowing payKeeperFromBalances to take an indiscriminate amount of funds may prove to be a rug vector the sponsor should consider eliminating.

I'm fairly confident the math library will never revert, even if you input a high fee, which means that while the end state may be unexpected, the function can be used to rug.

Highly recommend the sponsor to consider having caps on the amount parameter for payKeeperFromBalances as this function seems to be the way to break the protocol (and the trust of the users)

[kumar-ish]

@GalloDaSballo Yep, that's completely right. However (and this is something we should have made clear in an assumptions section), all markets will be deployed by the DAO/DAO multisig. You can see that the payKeeperFromBalances function has a modifier called onlyGov -- meaning only governance (the DAO/DAO multisig) can change this and that's something that's immutable and can't be changed. Governance also has the ability to take quote tokens out of the LeveragedPool contract (with withdrawQuote) unlike the hack with changing the keeper (more directly this way) but we've been going under the assumption that the only reason we'd do this is in the rescue of user funds in the case of some hack.

If the governance multisig is compromised, they have the ability to do a lot of damage but we've been going under the assumption this just won't be the case, which I think is a safe one to make. So you're right in that uncommit can send tokens to the wrong user in case the function reverts because of governance draining funds directly or via setting a malicious keeper, but I think that uncommit sending tokens to the wrong user is the least of problems if that happens because governance would only ever drain funds in the case of a major hack. If we're going under the assumption that the multisig can easily be compromised, then the "centralisation" point around the DAO would be a much bigger point.

I still think this should be a low/information by virtue of the fact that unless there is an underlying bug in the contracts, uncommit can't be called (it may as well be dead code) -- if it gets called because governance has rugged, that is then relatively a fairly small problem because all the funds would be at risk in a much more direct way in that case.

[GalloDaSballo]

@kumar-ish agree that if governance is malicious, the _uncommit path is the least problem

One thing to note is that the keeper has the ability of trying to claim a lot of fees, and the modifier seems to be onlyKeeper if the keeper were to be a bot, or a human operator they would have the ability of rugging, unless the parameter amount was under some check (let's say less than 1% AUM or something)

I agree that you can set to the PoolKeeper contract which seems safe, once potential rug vector, again from governance would be to inject a high gasPrice via setGasPrice which as a onlyOwner modifier

Contract for PoolKeeper: https://arbiscan.io/address/0x759E817F0C40B11C775d1071d466B5ff5c6ce28e#code

The owner is the Dev Multisig: https://arbiscan.io/address/0x0f79e82ae88e1318b8cfc8b4a205fe2f982b928a#readContract

This does give the Dev Multisig admin privileges and a potential for griefing at the very least if not rugging, as they could raise the gas price, and then run performUpkeepSinglePool which would eventually call payKeeperFromBalances

That said, this is something I'm flagging up right now and outside of the contest

[GalloDaSballo]

As per the finding at this point I believe it's valid and at medium severity, it is not high severity due to need for existing preconditions that are not "usual", see definition from Gitbook: 2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

We can agree that the pre-condition here are drastic and this finding would be the last of the protocols problems

In terms of mitigation, ensuring that the funds are send back to the address that initiated the commit are more than sufficient

[GalloDaSballo]

Per the "multisig privileges" above, I recommend you replace owner on the poolKeeper with a TimeLock as in it's current state, the gasPrice may be a way to rug funds from depositors

Please note that findings in this repo are private and only you, I and CodeArena are aware of this