050.md

Overt Alabaster Cottonmouth

High

Missing check in unvouch() allows compromised or deleted address to steal balance

Description

The protocol comments the following inside unvouch():

    // because it's $$$, you can only withdraw/unvouch to the same address you used to vouch
    // however, we don't care about the status of the address's profile; funds are always attached
    // to an address, not a profile

But the protocol does not also check if the address belongs to the profile anymore.

This leads to the following vulnerability:

1. Alice vouches for Bob using address A1.
2. A1 gets compromised and Alice marks it as compromised. (Alternatively, A1 is deleted by Alice and A1 gets assigned to some other profile id).
3. The attacker who compromised A1 can still call unvouch().
4. The funds get sent back to A1, which the attacker now controls.
5. Alice has no recourse to block the attacker's actions or recover her funds.

Impact

Two impacts:

1. A compromised/deleted address is able to steal the funds via unvouch().
2. Even if a check is introduced to block such addresses from receiving funds, there exists no way to rescue these funds and return to the rightful author.

Proof of Concept

Apply the following patch inside test/EthosVouch.test.ts and see it pass when run via npm run hardhat -- test --grep "should allow compromised address to unvouch and receive funds":

diff --git a/ethos/packages/contracts/test/EthosVouch.test.ts b/ethos/packages/contracts/test/EthosVouch.test.ts
index be4d7f1..0424b29 100644
--- a/ethos/packages/contracts/test/EthosVouch.test.ts
+++ b/ethos/packages/contracts/test/EthosVouch.test.ts
@@ -1,10 +1,11 @@
 import { loadFixture, time } from '@nomicfoundation/hardhat-toolbox/network-helpers.js';
 import { expect } from 'chai';
 import hre from 'hardhat';
 import { smartContractNames } from './utils/mock.names.js';
+import { common } from './utils/common.js';
 
 const { ethers } = hre;
 
 describe('EthosVouch', () => {
   const DEFAULT_COMMENT = 'default comment';
   const DEFAULT_METADATA = '{ "someKey": "someValue" }';
@@ -1202,9 +1203,78 @@ describe('EthosVouch', () => {
             ),
           )
             .to.be.revertedWithCustomError(ethosVouch, 'NotAuthorForVouch')
             .withArgs(0, 5);
         });
       });
+       
+      describe('Vouch security', () => {
+          it('should allow compromised address to unvouch and receive funds', async () => {
+              const { 
+                  ethosProfile, 
+                  ethosVouch, 
+                  VOUCHER_0, 
+                  PROFILE_CREATOR_0,
+                  OTHER_0,
+                  EXPECTED_SIGNER,
+                  OWNER 
+              } = await loadFixture(deployFixture);
+
+              // Set up profile for voucher
+              await ethosProfile.connect(OWNER).inviteAddress(VOUCHER_0.address);
+              await ethosProfile.connect(VOUCHER_0).createProfile(1);
+
+              // Set up profile for subject
+              await ethosProfile.connect(OWNER).inviteAddress(PROFILE_CREATOR_0.address);
+              await ethosProfile.connect(PROFILE_CREATOR_0).createProfile(1);
+
+              // VOUCHER_0 creates a vouch for PROFILE_CREATOR_0
+              const vouchAmount = ethers.parseEther('100');
+              await ethosVouch.connect(VOUCHER_0).vouchByProfileId(
+                  3, // PROFILE_CREATOR_0's profile ID
+                  'Vouch comment',
+                  'Vouch metadata',
+                  { value: vouchAmount }
+              );
+
+              // Get VOUCHER_0's balance before their address is compromised
+              const voucherBalanceBefore = await ethers.provider.getBalance(VOUCHER_0.address);
+
+              // Now simulate VOUCHER_0's address being compromised
+              // First register a new address for their profile so they can mark original as compromised
+              const randValue = BigInt('29548234957'); // Using a deterministic value for testing
+              const signature = await common.signatureForRegisterAddress(
+                  OTHER_0.address,
+                  '2', // VOUCHER_0's profile ID
+                  randValue.toString(),
+                  EXPECTED_SIGNER
+              );
+              
+              await ethosProfile
+                  .connect(VOUCHER_0)
+                  .registerAddress(OTHER_0.address, 2, randValue, signature);
+
+              // Mark VOUCHER_0's original address as compromised using their new address
+              await ethosProfile.connect(OTHER_0).deleteAddress(VOUCHER_0.address, true);   // <------- Alternatively, can pass `false` as last param if only want to delete, without marking as compromised
+
+              // Verify the address is marked as compromised
+              expect(await ethosProfile.isAddressCompromised(VOUCHER_0.address)).to.be.true; // <------- comment this if not marked as compromised in the step above
+
+              // Despite being compromised, the address can still unvouch and receive funds
+              // Simulate the attacker who now controls VOUCHER_0's compromised address
+              const unvouchTx = await ethosVouch.connect(VOUCHER_0).unvouch(0);
+              await unvouchTx.wait();
+
+              // Check that the compromised address received the funds
+              const voucherBalanceAfter = await ethers.provider.getBalance(VOUCHER_0.address);
+              // The balance should have increased (minus gas costs)
+              expect(voucherBalanceAfter).to.be.closeTo(voucherBalanceBefore + ethers.parseEther('100'), ethers.parseEther('0.001'));
+
+              // Verify the vouch is now archived
+              const vouch = await ethosVouch.vouches(0);
+              expect(vouch.archived).to.be.true;
+          });
+      });
+
     });
   });
 });

Mitigation

1. Add the check to revert if a compromised/deleted author address attempts to be the recipient of unvouch():

  function unvouch(uint256 vouchId) public whenNotPaused nonReentrant {
    Vouch storage v = vouches[vouchId];
    _vouchShouldExist(vouchId);
    _vouchShouldBePossibleUnvouch(vouchId);
    // because it's $$$, you can only withdraw/unvouch to the same address you used to vouch
    // however, we don't care about the status of the address's profile; funds are always attached
    // to an address, not a profile
    if (vouches[vouchId].authorAddress != msg.sender) {
      revert AddressNotVouchAuthor(vouchId, msg.sender, vouches[vouchId].authorAddress);
    }
    
+   // get the profile id of the author
+    uint256 profileId = IEthosProfile(
+      contractAddressManager.getContractAddressForName(ETHOS_PROFILE)
+    ).verifiedProfileIdForAddress(msg.sender);
+    _vouchShouldBelongToAuthor(vouchId, profileId);

    v.archived = true;
    // solhint-disable-next-line not-rely-on-time
    v.activityCheckpoints.unvouchedAt = block.timestamp;
    // remove the vouch from the tracking arrays and index mappings
    _removeVouchFromArrays(v);

    // apply fees and determine how much is left to send back to the author
    (uint256 toWithdraw, ) = applyFees(v.balance, false, v.subjectProfileId);
    // set the balance to 0 and save back to storage
    v.balance = 0;
    // send the funds to the author
    // note: it sends it to the same address that vouched; not the one that called unvouch
    (bool success, ) = payable(v.authorAddress).call{ value: toWithdraw }("");
    if (!success) {
      revert FeeTransferFailed("Failed to send ETH to author");
    }

    emit Unvouched(v.vouchId, v.authorProfileId, v.subjectProfileId);
  }

2. The second fix is more of a design decision which the protocol can take. There should be a way to recover this fund on calling unvouch(). Either a new convenience function reassignVouchAddress() can be added which looks something like this:

function reassignVouchAddress(
    uint256 vouchId, 
    address newAddress,
    bytes calldata signature  // Signed proof of new address ownership
) external {

Or the admin could float a proposal which allows them to rescue funds from such compromised vouches, and introduce a new function for the same.