Cred creator could stuck funds

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/Cred.sol#L283-L310

Vulnerability details

Impact

Update of cred can be triggered by the cred creator. If there are already issued shares for users, and the cred creator decides to update the sellShareRoyalty_ this will lead users to pay up to 50% fee depending on the value. And users will be in situation that to withdraw their funds, they need to pay 50% to the cred creator and additionally some fee to the protocol.

Proof of Concept

We have the following situation:

1. 500 issued shares and the current sell fee is 0.05%
2. And the cred creator decides to increase the fee to 50%
3. If any of the users wants to withdraw funds, they need to pay 50% to the creator and also protocol fee

For example let's imagine that the current number of shares issued is 500 of max 999 and one of the users is owner of 5 shares. If we add this test to Cred.t.sol

function test_value() public {
        uint256 x = bondingCurve.getSellPrice(500, 5);
        assertEq(153_019_801_980_198_020, x);
    }

we will see that the sell price is around 1.53e17 which in the current price is equal to 420$, this means that the 50% or 210$ will be paid to the cred creator as fee, which will lead to significant lose of funds for share holders. If the number of shares that are issued the lose is growing.

Tools Used

Manual review

Recommended Mitigation Steps

The protocol should implement a mechanism where this type of updates, will be applied after some period. I will explain my idea in the below lines:

1. If cred creator wants to increase the sellShareRoyalty_, it should first create a request for updating which can be applied after 2 days for exapmles.
2. In these two days users who are not okay with the new value of sellShareRoyalty_ can withdraw their funds from the protocol.

My proposal is to have two functions

1. updateSellShareRoyaltyRequest which will create this request and this request can be approved after some period.
2. approveUpdateSellShareRoyaltyRequest if the period already passed the cred creator can apply the change.

Assessed type

Access Control

[ZaK3939]

The observation is valid. However, I believe the main issue is the ability to update the royalty percentage. The high percentage is deliberately set to allow creators to set up creds where they prefer to minimise sharing with others. Therefore, my plan is to keep the current high percentage limit, but remove the update functionality altogether. This vulnerability should be rated medium severity in my opinion.

[c4-judge]

fatherGoose1 changed the severity to 2 (Med Risk)

[fatherGoose1]

I agree that the functionality should be removed or placed behind a timelock. Medium.

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 marked the issue as selected for report

[mcgrathcoutinho]

Hi @fatherGoose1, I believe this issue should be of QA-severity.

The main intention behind creds is for users to form, visualize, showcase their onchain identity. When a cred is created, the creator i.e. the onchain identity is the source of trust when users buy up their shares. This means, the more social, reputed and trusted an identity is, the lower are the chances of a creator wanting to perform such an attack.

It is highly unlikely a high reputed creator would want to perform such an attack. When it comes to low reputed creators, who are more likely to perform this attack, the impact would be low because the shares being bought for such untrusted entities would not be high (Note: This claim is based on onchain data I've observed from a similar protocol). If we look at the price curve, even if 5-10 shares were bought by users, the overall loss would be minimum and bearable, considering that users know the risk when buying shares of untrusted onchain identities.

I think the above points along with the fact that 0-50% is an acceptable range defined by the protocol as per sponsor's comments above, it seems that this issue is rather suited for QA-severity.

[fatherGoose1]

This is a valid medium severity issue. Such a function that can directly affect user funds, especially through frontrunning, should be mitigated by a timelock or similar.