Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Magnificent Tortilla Eel - Incorrect updateDonationRecipient() Function. #143

Open
sherlock-admin2 opened this issue Dec 30, 2024 · 0 comments

Comments

@sherlock-admin2
Copy link

Magnificent Tortilla Eel

High

Incorrect updateDonationRecipient() Function.

Summary

There could be a situation where a user has multiple markets, leading to a loss of funds for the owner when updating the donation recipient.

Root Cause

https://github.com/sherlock-audit/2024-12-ethos-update/blob/main/ethos/packages/contracts/contracts/ReputationMarket.sol#L641

Internal pre-conditions

N/A

External pre-conditions

N/A

Attack Path

  1. Alice registers with profile1 and creates a market.
  2. Alice is deleted from profile1 and re-registers with profile2.
  3. Alice receive the ownership of market in profile2.
    Now, Alice is reciving donations from 2 markets.
    Assuming that the donation amount from profile1 is 2e18, and the donation amount from profile2 is 1e18.
  4. Alice is going to transfer ownership to Bob, who in profile1.
    At this time, Alice should send the ownership and 2e18.
    However, in L641, all of Alice's 3e18 are sent to Bob.

Impact

Loss of funds.

PoC

https://github.com/sherlock-audit/2024-12-ethos-update/blob/main/ethos/packages/contracts/contracts/EthosProfile.sol#L415

  • @notice Deleted addresses can be re-registered to any profile.

As you can see, the deleted address can be re-registered to any profile as long as it is not marked as Compromised.
The EthosProfile.sol is implemented this way.

ReputationMarket.sol
        function updateDonationRecipient(
            uint256 profileId,
            address newRecipient
        ) public whenNotPaused nonReentrant {
            if (newRecipient == address(0)) revert ZeroAddress();

            // if the new donation recipient has a balance, do not allow overwriting
            if (donationEscrow[newRecipient] != 0)
            revert InvalidMarketConfigOption("Donation recipient has balance");

            // Ensure the sender is the current donation recipient
            if (msg.sender != donationRecipient[profileId]) revert InvalidProfileId();

            // Ensure the new recipient has the same Ethos profileId
            uint256 recipientProfileId = _ethosProfileContract().verifiedProfileIdForAddress(newRecipient);
            if (recipientProfileId != profileId) revert InvalidProfileId();

            // Update the donation recipient reference
            donationRecipient[profileId] = newRecipient;
            // Swap the current donation balance to the new recipient
641:        donationEscrow[newRecipient] += donationEscrow[msg.sender];
642:        donationEscrow[msg.sender] = 0;
            emit DonationRecipientUpdated(profileId, msg.sender, newRecipient);
        }

Mitigation

Consider using both the recipient and profileId as parameters for the donationEscrow. This ensures that funds are correctly attributed to the specific profile and recipient combination, preventing potential loss of funds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant