Centrifuge - m_Rassska's results

The institutional ecosystem for on-chain credit.

General Information

Platform: Code4rena

Start Date: 08/09/2023

Pot Size: $70,000 USDC

Total HM: 8

Participants: 84

Period: 6 days

Judge: gzeon

Total Solo HM: 2

Id: 285

League: ETH

Centrifuge

Findings Distribution

Researcher Performance

Rank: 66/84

Findings: 1

Award: $12.79

QA:

grade-b

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository Centrifuge

Findings Information

🌟 Selected for report: castle_chain

Also found by: 0xAadi, 0xHelium, 0xLook, 0xblackskull, 0xfuje, 0xmystery, 0xnev, 0xpiken, 7ashraf, BARW, Bauchibred, Bughunter101, Ch_301, JP_Courses, Kaysoft, Krace, MohammedRizwan, SanketKogekar, Sathish9098, alexzoid, ast3ros, btk, catellatech, degensec, fatherOfBlocks, grearlake, imtybik, jkoppel, jolah1, klau5, lsaudit, m_Rassska, merlin, mrudenko, nobody2018, rokinot, rvierdiiev, sandy

Awards

12.7917 USDC - $12.79

Labels

bug

disagree with severity

downgraded by judge

grade-b

QA (Quality Assurance)

sponsor acknowledged

sufficient quality report

Q-20

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/main/src/LiquidityPool.sol#L220-L226

Vulnerability details

Vulnerability Details

The current version of assets in liquidity pools implements ERC-2612 on top of the ERC-20 standard in order to enable meta transactions. However, it opens a room for potential vulnerabilities under certain circumstances.

After seeing the rise of RWAs, Binance decides to step in and collaborate with Centrifuge's Governance in order to support some RWA pools for its customers. To make it happen, they create a proposal to add the LP with their own native currency(e.g. BNB) being as an underlying asset. Seeing the following proposal, Governance will approve it, since the BNB is backward compatible with the currencies being used in a system. After all adjusmets being made, users start to deposit into the pools and everything goes seamless as desined.

As you already know, some of the native gas tokens do not have the logic implemented for .permit() whatsoever, but they do have non-reverting fallback() in order to accept the native currency.

Having the context needed, we can finally look into the requestDepositWithPermit():

  function requestDepositWithPermit(uint256 assets, address owner, uint256 deadline, uint8 v, bytes32 r, bytes32 s) public 
    {
        ERC20PermitLike(asset).permit(owner, address(investmentManager), assets, deadline, v, r, s);
        investmentManager.requestDeposit(assets, owner);
        emit DepositRequested(owner, assets);
    }

In this case, the asset is BNB and upon invoking, it will simply redirect the flow with the dummy payload into the fallback() function which does nothing at all and does not revert, which is important. Since it does not revert, everything after ERC20PermitLike(asset).permit(...) will be processed further.

The question now is what kind of power do we have with such a trick. I would sayyyy: we can mess up almost everything on behalf of everyone.
- For instance:
  - It's possible to cancel every single non-finalized order, thus the system is halt.
  - It's possible to deposit on behalf of the owner(if he approves type(uint256).max to the LP before his first deposit)
  - It's possible to request redeem for an arbitrary owner on his behalf.
  - And many more ...

Impact

I'm not aware the cases, where theft is possible, but the community will face complete halt of the LPs and unnecessary migrations that could affect the reputation.

Proof of Concept

For the case with complete halt an attacker could simply:
- Back-run the tx of a potential investor with a cancel message, so it will be never executed at the end of an epoch.
For all other described cases, the PoC is intuitive.

Recommended Mitigation Steps

The Debaub team has white-hated $1b for a Multichain users by utilizing similar approach. In that case, Multichain has decided to completely remove those functions, which may not be ideal in the context of LPS.

Assessed type

Context

#0 - c4-pre-sort
2023-09-15T21:26:23Z

raymondfam marked the issue as sufficient quality report

#1 - c4-sponsor
2023-09-18T12:45:18Z

hieronx (sponsor) acknowledged

#2 - c4-sponsor
2023-09-18T12:46:01Z

hieronx marked the issue as disagree with severity

#3 - hieronx
2023-09-18T12:46:04Z

Acknowledged but very unlikely, and still protected through both currency addition checks and the fact that it would first need to be deployed on a centralized L1/L2 for this to even be feasible.

#4 - gzeon-c4
2023-09-25T11:22:55Z

Potential to DOS via backrunning with assets=0 to cancel the deposit request
Potential to force deposit if unlimited approve is given to the contract
Does not seems to be able to steal or otherwise make asset stuck in the protocol
Centrifuge is designed to handle stablecoin according to the README, and there isn't a well-adopted stablecoin that have a non-reverting permit function; it is the governance's responsibility to review any token to be used

I believe this is out-of-scope due to (4) but I think this is something nice to be documented. Downgrading to Low/QA.

#5 - c4-judge
2023-09-25T11:23:03Z

gzeon-c4 changed the severity to QA (Quality Assurance)

#6 - c4-judge
2023-09-25T11:23:12Z

gzeon-c4 marked the issue as grade-a

#7 - c4-judge
2023-09-26T18:27:51Z

gzeon-c4 marked the issue as grade-b

#8 - Rassska
2023-09-28T00:45:14Z

Hey, @gzeon-c4, kudos for the feedback 🎉 I don't mean to undermine the judging, but rather will provide additional context for you to evaluate under better angle.

Does not seems to be able to steal or otherwise make asset stuck in the protocol

As of this, I know, I haven't included any details on why the system is completely halt by saying the following:

It's possible to cancel every single non-finalized order, thus the system is halt.

The system is halt, since no deposits are being made => solver mechanism has to be initiated in order to get the submission under a defined set of restrictions for the linear maximization problem. Since the deposits are paused, it's unlikely that the solver will provide a solution without breaking those restrictions, because in order to finalize withdrawal requests the system rely on its reserves + new investments.

Centrifuge is designed to handle stablecoin according to the README, and there isn't a well-adopted stablecoin that have a non-reverting permit function; it is the governance's responsibility to review any token to be used

With this, I 100% agree with you, but it's not like: adding fee-on-transfer token will be break the system. Since in this case, both Governance and the devs behind Centrifuge clearly are aware about the limits behind the system.

But in a case, with wrapped versions of desentralized gas tokens, the system actually works as intended => it's compatible to operate with those tokens, however, it opens a room for a potential exploit described above.

The impact here is critical, however, the likelihood is low/medium. Based on the impact and the likelihood I believe the severity could be raised up to the "Medium". The similar evaluation has been handled here: https://github.com/code-423n4/2023-07-axelar-findings/issues/267#issuecomment-1713738431 , but in this case the critical impact occurs, in case of deployer's EOA being compromised.

@hieronx , sorry for tagging, but your opinion is also much appreciated to fairly decide the contribution behind this issue!!!

Thanks a lot for looking into it! I just simply wanna make sure, that this issue will be clearly documented in the report, so that the devs and Governance members will be aware of it.

#9 - gzeon-c4
2023-09-28T09:46:25Z

I still believe this is low risk as this require the governance to add an incompatible token, and there is a lot of way to create a token that is incompatible with various part of centrifuge. The Centrifuge chain may also have the ability to ignore any malicious deposit cancelation request as it is a blackbox.

#10 - Rassska
2023-09-28T09:54:19Z

I still believe this is low risk as this require the governance to add an incompatible token, and there is a lot of way to create a token that is incompatible with various part of centrifuge. The Centrifuge chain may also have the ability to ignore any malicious deposit cancelation request as it is a blackbox.

Gotcha, thanks a lot!

Centrifuge - m_Rassska's results

The institutional ecosystem for on-chain credit.

General Information

Findings Distribution

Researcher Performance

External Links

[Q-20] QA Report - $12.79

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Vulnerability Details

Impact

Proof of Concept

Recommended Mitigation Steps

Assessed type

#0 - c4-pre-sort2023-09-15T21:26:23Z

#1 - c4-sponsor2023-09-18T12:45:18Z

#2 - c4-sponsor2023-09-18T12:46:01Z

#3 - hieronx2023-09-18T12:46:04Z

#4 - gzeon-c42023-09-25T11:22:55Z

#5 - c4-judge2023-09-25T11:23:03Z

#6 - c4-judge2023-09-25T11:23:12Z

#7 - c4-judge2023-09-26T18:27:51Z

#8 - Rassska2023-09-28T00:45:14Z

#9 - gzeon-c42023-09-28T09:46:25Z

#10 - Rassska2023-09-28T09:54:19Z

#0 - c4-pre-sort
2023-09-15T21:26:23Z

#1 - c4-sponsor
2023-09-18T12:45:18Z

#2 - c4-sponsor
2023-09-18T12:46:01Z

#3 - hieronx
2023-09-18T12:46:04Z

#4 - gzeon-c4
2023-09-25T11:22:55Z

#5 - c4-judge
2023-09-25T11:23:03Z

#6 - c4-judge
2023-09-25T11:23:12Z

#7 - c4-judge
2023-09-26T18:27:51Z

#8 - Rassska
2023-09-28T00:45:14Z

#9 - gzeon-c4
2023-09-28T09:46:25Z

#10 - Rassska
2023-09-28T09:54:19Z