setRewardRecipient function allows anyone to set any address as reward recipient address

[hats-bug-reporter[bot]]

Github username: -- Submission hash (on-chain): 0xfeb145e868a5fb32fa5b6995bf661f690941ba072b79e70d3b6b047e0e7c842c Severity: high

Description: Description\ The issue is that msg.sender is used to set the rewardRecipient mapping, but there is no validation that msg.sender is actually a registered withdrawal address.

This means any arbitrary address could call setRewardRecipient and set the reward recipient for any withdrawal address in the mapping.

For example: • Attacker calls setRewardRecipient(attackerAddress) • This sets rewardRecipient[msg.sender] = attackerAddress • Attacker address is now recipient for any real withdrawal address

Attachments https://github.com/dappnode/mev-sp-contracts/blob/3929e24ea288d697d38948b8690c8c2028e5042b/contracts/DappnodeSmoothingPool.sol#L342-L345

1. Proof of Concept (PoC) File

   /**

   • @notice Allow a withdrawal address to set a reward recipient
   • @param rewardAddress Reward recipient */ function setRewardRecipient(address rewardAddress) external { rewardRecipient[msg.sender] = rewardAddress; emit SetRewardRecipient(msg.sender, rewardAddress); }

2. Revised Code File (Optional)

[invocamanman]

I don't understand this recipient for any real withdrawal address But the point of this function is that a withdrawal address that must be controlled ( bc the msg.sender is checked) can define an address to receive the rewards from the pool instead of himself. You cannot set the rewards recipient of an arbitrary withdrawal address, you can only set the ones that you actually control, bc is only setted the msg.sender

[ololade97]

@invocamanman, what I mean is this:

If msg.sender is not in the rewardRecipient mapping, any msg.sender can call the function and set their own rewardAddress in the mapping.

And this shouldn't be the case.

[ololade97]

*set his own...

[ololade97]

Here's a Foundry test:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";

contract RewardRecipient {
    mapping(address => address) public rewardRecipient;

    event SetRewardRecipient(address withdrawalAddress, address poolRecipient);

    function setRewardRecipient(address rewardAddress) external {
        rewardRecipient[msg.sender] = rewardAddress;
        emit SetRewardRecipient(msg.sender, rewardAddress);
    }
}

contract RewardRecipientTest is Test {
    RewardRecipient rewardrecipient;

    address randomUser = vm.addr(1);
    address recipient = vm.addr(2);

function setUp() public {
  rewardrecipient = new RewardRecipient(); 
}

    function testsetRewardRecipient() public {
        vm.startPrank(randomUser);
        rewardrecipient.setRewardRecipient(recipient);
        vm.stopPrank();
    }
}

[ololade97]

Here's the result:

forge test --mc RewardRecipientTest -vvvv [⠔] Compiling... [⠘] Compiling 1 files with 0.8.21 [⠃] Solc 0.8.21 finished in 1.17s Compiler run successful!

Running 1 test for test/Reentrancy.t.sol:RewardRecipientTest [PASS] testsetRewardRecipient() (gas: 36421) Traces: [36421] RewardRecipientTest::testsetRewardRecipient() ├─ [0] VM::startPrank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf) │ └─ ← () ├─ [23812] RewardRecipient::setRewardRecipient(0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF) │ ├─ emit SetRewardRecipient(withdrawalAddress: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, poolRecipient: 0x2B5AD5c4795c026514f8317c7a215E218DcCD6cF) │ └─ ← () ├─ [0] VM::stopPrank() │ └─ ← () └─ ← ()

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.03ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

[ololade97]

But the point of this function is that a withdrawal address that must be controlled ( bc the msg.sender is checked) can define an address to receive the rewards from the pool instead of himself. You cannot set the rewards recipient of an arbitrary withdrawal address, you can only set the ones that you actually control, bc is only setted the msg.sender

The problem is that anyone can set reward address by calling the setRewardRecipient function, as shown in the test. Anyone shouldn't be able to do this.

[invocamanman]

Yes anyone can set his own reward recipient, what's wrong with that? Be aware that there's a reward recipient for every address! maybe here it is the confusion?

[ololade97]

What is wrong is that a malicious user can:

• Bloat the storage by repeatedly calling it to set different recipients for various addresses.

The rewardRecipient mapping stores data in contract storage. Each new msg.sender => rewardAddress pair that is set will take up additional storage.

An attacker can exploit this by calling setRewardRecipient in a loop across many different msg.sender and rewardAddress values.

Even though these addresses may not be "real" user accounts, it will still write data to contract storage. By repeatedly calling with unique address pairs, the attacker can quickly bloat storage with meaningless data. This could cause the contract to hit gas limits for storage, prevent legitimate data storage, or drive up gas costs for users.

[invocamanman]

Floading a storage of a contract which is not used is not making anything more expensive in gas terms. In the case that you expose, the attacker basically is burning gas to write to some storage slots that they will remain unchecked/unused. Also, you can do this kind of "floading" in A LOT of contracts. If the purpose is to just write some random data in an unused storage. For example, take ANY erc20 token, you can call approve in some arbitrary account that not necessary have tokens, to approve some arbitrary token amount to another arbitrary address. You are not harming an erc20 in any way, by "floading" the approval mappings

Well.. actually you could harm! if you find a collision on the keccak256 ;) ( but ofc this would have further more implication like impersonating signatures on ethereum itself...)

[ololade97]

It's even possible for an attacker to think of other ways of breaching the function and the contract in general we are not thinking about right now. The best security approach is to limit the function to legitimate users.