search

code-423n4 / 2023-07-tapioca-findings

15 stars 10 forks source link

`Penrose.setUsdoToken()` may cause users funds to be lost #1028

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/a3b45512580f8a76be45c19f635689f48c0128c3/contracts/LiquidationQueue/LiquidationQueue.sol#L112 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/a3b45512580f8a76be45c19f635689f48c0128c3/contracts/LiquidationQueue/LiquidationQueue.sol#L245 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/a3b45512580f8a76be45c19f635689f48c0128c3/contracts/LiquidationQueue/LiquidationQueue.sol#L259 https://github.com/Tapioca-DAO/tapioca-periph-audit/blob/023751a4e987cf7c203ab25d3abba58f7344f213/contracts/LiquidationQueue/LiquidationQueue.sol#L406-L412 https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/Penrose.sol#L302

Vulnerability details

Impact

Governance can call Penrose.setUsdoToken() to change the Penrose.usdoToken and Penrose.usdoAssetId state variables anytime.

This can cause users to lose their funds.

Example:

  1. Alice adds their bid to the liquidation queue of a USDO-Singularity market via LiquidationQueue.bidWithStable(). They transfer some USDO for their bid to the LiquidationQueue (line 259 in LiquidationQueue.sol).

  2. Then Governance calls Penrose.setUsdoToken() on the Penrose market from the LiquidationQueue (line 112 LiquidationQueue.sol), where a new usdoAssetId is set (line 302 Penrose.sol).

  3. Now Alice wants to remove their bid via LiquidationQueue.removeBid(), but since the usdoAssetId was changed by Governance, there is an issue on line 406-412 in LiquidationQueue.sol where the protocol wants to transfer back the USDO to Alice. The assetId is fetched via penrose.usdoAssetId which is now different since it was changed (line 406 LiquidationQueue.sol). The transfer tx (line 407 LiquidationQueue.sol) to send back the funds to Alice then potentially fails, since there may be no balance available in the YieldBox for the LiquidationQueue for the new usdoAssetId. Or if there would be balance available, it would be "stolen" by Alice since Alice never transferred any amount of the new usdoAssetId to the liquidation queue. So another bidder who transferred USDO to the LiquidationQueue after Governance called Penrose.setUsdoToken() might lose their funds when Alice removes her bid. The funds that were transferred to the LiquidationQueue via bids before Governance called Penrose.setUsdoToken() are trapped in the LiquidationQueue.

As a consequence of this issue, LiquidationQueue.removeBid() may always revert after Governance changed the USDO token via Penrose.setUsdoToken() as shown above in the example. Also instead of the revert condition, funds might get lost in the LiquidationQueue, since the usdoAssetId was changed because Governance called Penrose.setUsdoToken().

Proof of Concept

The POC shows that after Governance calls Penrose.setUsdoToken(), the bid that occured before can't be removed any more because LiquidationQueue.removeBid() reverts:

https://gist.github.com/zzzitron/629e74c3f12ec27f489dc43282bf9eee

Tools Used

Manual review

Recommended Mitigation Steps

Consider making the Penrose.usdoAssetId unchangeable when Governance calls Penrose.setUsdoToken().

Assessed type

Other

c4-pre-sort commented 1 year ago

minhquanym marked the issue as low quality report

minhquanym commented 1 year ago

onlyOnwer function. Overinflated severity

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Overinflated severity