Reentrancy and untrusted call can be used to drain funds from claimRewards()

[hats-bug-reporter[bot]]

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

Description: Description\ Missing checks and no re-entrancy prevention allow untrusted contracts to call claimRewards(). This could be used by an attacker to drain the reward.

Attack Scenario\ A staker can turn malicious by creating a custom contract that will call the claimRewards(). The attacker is free to do as he pleases within this function as it is an external contract callable by anyone.

Attachments

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

1. Proof of Concept (PoC) File

There are no reentrancy guards in the DappnodeSmoothingPool contract and thus a malicious custom contract could call back into the claimRewards by taking advanatge of rewardAddress.call method on L#327.

The malicious call can be used to drain a major or whole part of the reward.

    function claimRewards(
        address withdrawalAddress,
        uint256 accumulatedBalance,
        bytes32[] memory merkleProof
    ) external {
        // Verify the merkle proof
        bytes32 node = keccak256(
            abi.encodePacked(withdrawalAddress, accumulatedBalance)
        );

        require(
            MerkleProofUpgradeable.verify(merkleProof, rewardsRoot, node),
            "DappnodeSmoothingPool::claimRewards: Invalid merkle proof"
        );

        // Get claimable ether
        uint256 claimableBalance = accumulatedBalance -
            claimedBalance[withdrawalAddress];

        // Update claimed balance mapping
        claimedBalance[withdrawalAddress] = accumulatedBalance;

        // Load first the reward recipient for gas saving, to avoid load twice from storage
        address currentRewardRecipient = rewardRecipient[withdrawalAddress];
        address rewardAddress = currentRewardRecipient == address(0)
            ? withdrawalAddress
            : currentRewardRecipient;

        // Send ether
        (bool success, ) = rewardAddress.call{value: claimableBalance}(
            new bytes(0)
        );
        require(
            success,
            "DappnodeSmoothingPool::claimRewards: Eth transfer failed"
        );

        emit ClaimRewards(withdrawalAddress, rewardAddress, claimableBalance);
    }

If there been re-entrancy protection placed, the attacker contract would not be able to call back into the contract as it will severely limit his abilities to do so.

2. Revised Code File (Optional)

Add Openzeppelin’s ReentrancyGuardUpgradeable re-entrancy guard modifier nonReentrant to all external functions that are callable by anyone specifically, to the claimRewards method.

[invocamanman]

It's true that there's an open reentrancy, but this is contemplated and cannot make any damage to the system, since all the state changes are made before the call begins, so, the smart contract is in a consistent state. Take in mind that the only thing that happens after the call is emit an event, which does not change the state of the smart contract in any way, and also would not be any problem when synching events even if the reentrancy is used.