048.md

Orbiting Walnut Ant

Medium

A compromised address can keep selling votes

Summary

A missing verifiedProfileIdForAddress check in ReputationMarket::sellVote will allow an attacker to sell votes with a compromised address.

Root Cause

The function sellVotes allows to sell trust or distrust votes from a market. The proceeds are then sent to the seller after fees. In case when a user's private key is stolen, an attacker can call this function to retrieve funds on behalf of the legitimate user. The protocol gives user the possibility to mark an address as compromised as fast as possible by calling deleteAddress, however, it won't stop the attacker from acquiring user's funds by selling votes as the msg.sender is never checked if it were compromised. In addition, all votes owned by a user are recorded inside the array votesOwned without being able to transfer them to a new address. As a result, when a market is graduated and the conversion is implemented, user position will be at risk as the equivalent ERC-20 tokens will still be transferred to the compromised address.

Internal Pre-conditions

An address is marked as compromised by the protocol.

External Pre-conditions

No response

Attack Path

1. Attacker steals Alice's private key
2. Alice detects it and calls deleteAddress immediately in order to revoke the compromised address
3. Attacker can keep selling all votes (trust or distrust) and withdrawing all donations from a market possessed by Alice

Impact

The protocol can't properly protect users in case of compromised addresses even if the address has been declared compromised. This results in fund losses and user frustration as the attacker is able to gain access to restricted operations.

PoC

No response

Mitigation

Add the verifiedProfileIdForAddress check in the sellVotes function. The following diff can be applied to the function sellVotes in the file ReputationMarket.sol:

function sellVotes(
   uint256 profileId,
   bool isPositive,
   uint256 votesToSell,
   uint256 minimumVotePrice
 ) public whenNotPaused activeMarket(profileId) nonReentrant {
   _checkMarketExists(profileId);
+   uint256 senderProfileId = _ethosProfileContract().verifiedProfileIdForAddress(msg.sender);
+   if (senderProfileId != profileId) revert InvalidProfileId();
   (uint256 proceedsBeforeFees, uint256 protocolFee, uint256 proceedsAfterFees) = _calculateSell(
     markets[profileId],
     profileId,
     isPositive,
     votesToSell
   );


   uint256 pricePerVote = votesToSell > 0 ? proceedsBeforeFees / votesToSell : 0;
   if (pricePerVote < minimumVotePrice) {
     revert SellSlippageLimitExceeded(minimumVotePrice, pricePerVote);
   }


__snip____snip____snip____snip____snip____snip____snip____snip____snip____snip____snip__

In addition, the protocol should introduce a new function to migrate votes owned by a compromised address inside the array votesOwned to a new one.

Note that when detecting a compromised address, user must call updateDonationRecipient as soon as possible in order to transfer any existing donation balance from the compromised address to a new one before the attacker's call to 'withdrawDonations'. The attacker can also call buyVotes using the compromised address, however, there is no incentive for him to do so. As a recommendation, the verifiedProfileIdForAddress check can also be added to the functions updateDonationRecipient and buyVotes.