Submitted by thekmj, also found by immeas, bin2chen and ast3ros.
https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/utilities/poolsUtility/Position.sol#L210-L223 https://github.com/code-423n4/2023-04-rubicon/blob/main/contracts/utilities/poolsUtility/Position.sol#L382-L397
The Compound V2 protocol is a lending protocol that allows users to supply collaterals in order to borrow other assets (possibly the collateral itself). If a users position gets undercollateralized, the user will get liquidated (i.e. a liquidator can seize some of their collateral, while repaying also part of their debt.)
The problem is that, if a users created margin position gets liquidated, it is not possible to close said position, at least not without disrupting other positions alongside that.
This is because the function closePosition() will attempt to close the position using the full amount it has been provided when opening. In other words, the contract never got the info that any liquidations happened, and will attempt to explicitly undo the position in addition to closing it.
We will use test/hardhat-tests/leverage-wrapper.ts for the POC.
https://github.com/code-423n4/2023-04-rubicon/blob/main/test/hardhat-tests/leverage-wrapper.ts#L194-L203
It is worth noting that the funds can still be recovered. This is because Compound exposes a function, repayBehalf, allowing the user to repay on behalf of Position, and Position itself exposes a function, withdraw, allowing users to withdraw their CTokens directly and redeem.
However, this defeats the purpose of Position contract, when the users are forced to manage their margin positions for the Position contract, as opposed to the contract doing so for them.
The most serious impact here, we believe, is not the difficult recover of funds, but in affecting/misleading user decisions when managing their margin portfolio. During a liquidation, an asset is repaid to seize another asset. If two assets that are seized belongs to two different positions, a technically liquidated position can be withdrawn, but leaving other positions at risk. In other words, the impossibility of closing a position will cause mismanagement, which is the opposite of what Position is trying to achieve.
Manual review + Hardhat for POC/testing
This is a very difficult problem to address due to its nature as a business logic problem. At the very least, it is not possible to tell whether a position was liquidated, due to the way cross-margin positionings work.
We suggest exposing the following extra functionalities to address the immediate problem:
HickupHH3 (judge) commented:
