c4-bot-9 commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/main/src/Cred.sol#L570

Vulnerability details

Impact

When a cred is created the caller is minted only the first credShare, shown here:

        buyShareCred(credIdCounter, 1, 0);

After this anyone is free to buy any quantity of shares they wish via Cred::buyShareCred. This causes a problem where should a cred that is deemed to be popular is created a sniper (typically a bot) can jump in and instantly buy a large amount of the shares for the newly created credId at the cheapest possible price. This is a scenario that has been a common problem for other popular projects that offer shares priced on a bonding curve such as Friend Tech which had significant issues with botting of newly created tokens. Here is a write-up documenting this activity on Friend Tech. (Note that while the thread talks about the sniping as "MEV" the same action could still be acheived on chains without public mempools by simply acting whenever the CredCreated event is emitted, which would be enough to front run everyday users.

Additionally a similar finding to this was highlighted in the earlier Curves Code4rena contest here.

Proof of Concept

The sniper waits for CredCreated event emissions and bulk buys tokens with notable creator_s (could use eth balance of creator_, ENS name, whatever other info they can get from the creators address).
If the credId is popular and users buy more cred shares, the sniper stands to make reasonable profit selling their cred shares as the bonding curve price increases.
If the credId is not popular the sniper can sell their shares making only a small loss (the protocol and creator fees paid), while still receiving the bulk of curator rewards for any art claimed that is linked to the credId.
Therefore the risk/reward for snipers is very favourable.

Recommended Mitigation

It is recommended that the cred creator should be allowed to specify how many shares they wish to buy when calling Cred::createCred as them buying more than just one share would negatively affect the risk/reward taken on by potential snipes in two ways:

Their price per token when buying in bulk would increase.
The cred creator would have more tokens available to potentially sell after the sniper's bulk buy, which would increase their downside risk considerably.

Assessed type

Other

McCoady commented 2 months ago

The negative effect of bot snipers in protocols that sell shares/tokens on a bonding curve is something well documented (with data on the values extracted by them included in this issue) so I'd request this issue be reconsidered especially as the same issue has been considered a valid medium severity finding in previous contests here.

fatherGoose1 commented 1 month ago

While I agree with your recommendation, it seems that the caller could simply create the cred through a contract, which could then call buyShareCred() after the cred was created.

@ZaK3939 do you specifically intend for the cred creator not to be able to "presale" a large quantity of creds to themselves? What was your intention here?

McCoady commented 1 month ago

While I agree with your recommendation, it seems that the caller could simply create the cred through a contract, which could then call buyShareCred() after the cred was created.

I agree using a contract would be a possible way for the cred creator to get around it, but it would require you to get a signature from the PhiSignerAddress where the contract's address is sender rather than the account that is interacting with the frontend, I'm not sure how feasible this would be as it depends on how the Phi frontend builds out signature data for users.

From Cred::createCred:

        (
            uint256 expiresIn,
            address sender,
            ,
            address bondingCurve,
            string memory credURL,
            string memory credType,
            string memory verificationType,
            bytes32 merkleRoot_
        ) = abi.decode(signedData_, (uint256, address, uint256, address, string, string, string, bytes32));
        if (expiresIn <= block.timestamp) revert SignatureExpired();

        if (sender != _msgSender()) revert Unauthorized();

ZaK3939 commented 1 month ago

@McCoady @fatherGoose1 I believe it would be very difficult for snipers to determine which credits are likely to be popular. We could change the system to allow free choice in purchase quantities, but I think it would be extremely challenging for snipers to target potentially popular creds as pointed out in this issue. Without art being set up, fees haven't accumulated, and there's a high likelihood that snipers would actually lose money instead.

Also, there is no specific intention behind allowing only one share to be purchased. However, by setting it at least one share, we are slightly mitigating the creation of unnecessary spam creds.