Complete halt under certain circumstances

[c4-submissions]

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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-sponsor]

hieronx (sponsor) acknowledged

[c4-sponsor]

hieronx marked the issue as disagree with severity

[hieronx]

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.

[gzeon-c4]

1. Potential to DOS via backrunning with assets=0 to cancel the deposit request
2. Potential to force deposit if unlimited approve is given to the contract
3. Does not seems to be able to steal or otherwise make asset stuck in the protocol
4. 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.

[c4-judge]

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

[c4-judge]

gzeon-c4 marked the issue as grade-a

[c4-judge]

gzeon-c4 marked the issue as grade-b

[Rassska]

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: code-423n4/2023-07-axelar-findings#267 (comment) , 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.

[gzeon-c4]

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.

[Rassska]

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!