code-423n4 / redacted-bug-bounty

13 stars 9 forks source link

This could potentially lead to a re-entrancy attack. #72

Closed c4-bot-3 closed 4 months ago

c4-bot-3 commented 4 months ago

Lines of code

https://github.com/redacted-cartel/pirex-eth-contracts/blob/11f30c7e35b67d45deefe405c22a30f352bc5b21/src/PirexEthValidators.sol#L887

Vulnerability details

I found a vulnerability related to the _deposit() function. The vulnerability lies in the fact that the function does not perform any checks on the beaconChainDepositContract before calling its deposit() function. This could potentially lead to a re-entrancy attack. M Here's a detailed explanation of the vulnerability and how to protect against it:

  1. Vulnerability Explanation: The _deposit() function calls the deposit() function of the beaconChainDepositContract without checking its contract status or performing any access control. An attacker could exploit this by creating a malicious contract that impersonates the beaconChainDepositContract and implements a re-entrancy attack. The attacker would then deploy the malicious contract and call the _deposit() function, allowing the malicious contract to execute arbitrary code within the PirexEthValidators contract.

  2. How to Protect Against It: To protect against re-entrancy attacks, you can use the ReentrancyGuard library, which is already imported in the PirexEthValidators contract. To do this, you need to inherit the ReentrancyGuard library in the PirexEthValidators contract and call the guarded() function before calling any external functions. In this case, you should modify the _deposit() function as follows:

function _deposit() internal guarded(guard) {
    // Existing code of the _deposit() function
}

By adding the guarded(guard) modifier, you ensure that the _deposit() function cannot be called while it is still executing, preventing potential re-entrancy attacks.

Additionally, you should include access control checks to ensure that only authorized contracts can call the _deposit() function. For example, you can use the OpenZeppelin AccessControl library to define roles and permissions. This will help you ensure that only the beaconChainDepositContract (or other authorized contracts) can call the _deposit() function.

In summary, to protect the PirexEthValidators contract from re-entrancy attacks in the _deposit() function, inherit the ReentrancyGuard library and use the guarded() modifier. Also, implement access control checks to ensure that only authorized contracts can call the _deposit() function. To protect the smart contract from re-entrancy attacks in the _deposit() function, you should follow these steps:

  1. Inherit the ReentrancyGuard library in the PirexEthValidators contract.
  2. Modify the _deposit() function by adding the guarded(guard) modifier to prevent re-entrancy attacks.
  3. Implement access control checks to ensure that only authorized contracts can call the _deposit() function.

Here's the updated _deposit() function with the guarded(guard) modifier:

function _deposit() internal guarded(guard) {
    uint256 remainingCount = maxProcessedValidatorCount;
    uint256 _remainingdepositAmount = DEPOSIT_SIZE - preDepositAmount;

    while (
        _initializedValidators.count() != 0 &&
        pendingDeposit >= _remainingdepositAmount &&
        remainingCount > 0
    ) {
        // Get validator information
        (
            bytes memory _pubKey,
            bytes memory _withdrawalCredentials,
            bytes memory _signature,
            bytes32 _depositDataRoot,
            address _receiver
        ) = _initializedValidators.getNext(withdrawalCredentials);

        // Make sure the validator hasn't been deposited into already
        // to prevent sending an extra eth equal to `_remainingdepositAmount`
        // until withdrawals are allowed
        if (status[_pubKey] != DataTypes.ValidatorStatus.None)
            revert Errors.NoUsedValidator();

        (bool success, ) = beaconChainDepositContract.call{
            value: _remainingdepositAmount
        }(
            abi.encodeCall(
                IDepositContract.deposit,
                (
                    _pubKey,
                    _withdrawalCredentials,
                    _signature,
                    _depositDataRoot
                )
            )
        );

        assert(success);

        pendingDeposit -= _remainingdepositAmount;

        if (preDepositAmount != 0) {
            _mintPxEth(_receiver, preDepositAmount);
        }

        unchecked {
            --remainingCount;
        }

        status[_pubKey] = DataTypes.ValidatorStatus.Staking;

        _stakingValidators.add(
            DataTypes.Validator(
                _pubKey,
                _signature,
                _depositDataRoot,
                _receiver
            ),
            _withdrawalCredentials
        );

        emit ValidatorDeposit(_pubKey);
    }
}

In order to implement access control checks, you can use the OpenZeppelin AccessControl library to define roles and permissions. This will help ensure that only the beaconChainDepositContract (or other authorized contracts) can call the _deposit() function.

For example, you can define a role for the beaconChainDepositContract and use the hasRole() function to check if the calling contract has the required role before allowing the _deposit() function to execute. This will ensure that only authorized contracts can call the _deposit() function, providing an additional layer of security against unauthorized access.

c4-bot-1 commented 4 months ago

Discord id(s) for hunter(s): [object Object]

MiloTruck commented 4 months ago

The beacon chain contract is not user-controlled.