code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L323 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L184

Vulnerability details

Impact

The comments on the affected lines state previewWithdraw will round up. However, the implementation, an inner call to convertToShares is made, which actually calls mulDivDown.

From further inspection, this pair of functions withdraw, previewWithdraw as well as the "rounds up" comment exists in both the base PirexERC4626 as well as AutoPxGmx, but the latter overrides it to slightly change the logic. The superclass implementation doesn't suffer from this problem, since the original previewWithdraw directly calls mulDivUp, and does not delegate to convertToShares

This has all the code smells of a copy&paste oversight.

Additionally, the second previewWithdraw implementation does one additional mathematical division that implicitly rounds down a second time.

Also, it must be said that this fees like a fragile solution. Why implement hooks on the base ERC4626 contract if those hooks are then not enough to prevent overriding whole functions in your final contracts?

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Recommended Mitigation Steps

It is unclear wether this constitutes a serious issue, or just potential for dust amounts to be left on the contract. Best case scenario: the comment is actually what's wrong, and should just be ammended to reflect the current behaviour