Skip to content

Latest commit

 

History

History
150 lines (123 loc) · 7.58 KB

File metadata and controls

150 lines (123 loc) · 7.58 KB

Reth poolCanDeposit is missing the validation to check if Rocket Pool deposits are enabled

Impact

The ability to stake ETH in the Rocket Pool protocol depends on certain external conditions related to the state of the protocol's contracts. The Reth derivative deals with this by checking if the deposit amount can be handled by Rocket Pool and switching to swapping ETH for RETH if not.

The conditions to switch between the two paths are implemented in the poolCanDeposit function. This function check if the deposit amount is above the required minimum and if the current pool balance plus the current deposit value doesn't exceed the maximum allowed by the protocol:

https://github.com/code-423n4/2023-03-asymmetry/blob/main/contracts/SafEth/derivatives/Reth.sol#L120-L150

function poolCanDeposit(uint256 _amount) private view returns (bool) {
    address rocketDepositPoolAddress = RocketStorageInterface(
        ROCKET_STORAGE_ADDRESS
    ).getAddress(
            keccak256(
                abi.encodePacked("contract.address", "rocketDepositPool")
            )
        );
    RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
            rocketDepositPoolAddress
        );

    address rocketProtocolSettingsAddress = RocketStorageInterface(
        ROCKET_STORAGE_ADDRESS
    ).getAddress(
            keccak256(
                abi.encodePacked(
                    "contract.address",
                    "rocketDAOProtocolSettingsDeposit"
                )
            )
        );
    RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(
            rocketProtocolSettingsAddress
        );

    return
        rocketDepositPool.getBalance() + _amount <=
        rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() &&
        _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit();
}

These two conditions are correct, however the function is missing another condition. As we can see in the following snippet from the deposit function of the RocketDepositPool contract:

https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/deposit/RocketDepositPool.sol#L74-L91

function deposit() override external payable onlyThisLatestContract {
    // Check deposit settings
    RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(getContractAddress("rocketDAOProtocolSettingsDeposit"));
    require(rocketDAOProtocolSettingsDeposit.getDepositEnabled(), "Deposits into Rocket Pool are currently disabled");
    require(msg.value >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit(), "The deposited amount is less than the minimum deposit size");
    RocketVaultInterface rocketVault = RocketVaultInterface(getContractAddress("rocketVault"));
    require(rocketVault.balanceOf("rocketDepositPool").add(msg.value) <= rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize(), "The deposit pool size after depositing exceeds the maximum size");
    // Calculate deposit fee
    uint256 depositFee = msg.value.mul(rocketDAOProtocolSettingsDeposit.getDepositFee()).div(calcBase);
    uint256 depositNet = msg.value.sub(depositFee);
    // Mint rETH to user account
    RocketTokenRETHInterface rocketTokenRETH = RocketTokenRETHInterface(getContractAddress("rocketTokenRETH"));
    rocketTokenRETH.mint(depositNet, msg.sender);
    // Emit deposit received event
    emit DepositReceived(msg.sender, msg.value, block.timestamp);
    // Process deposit
    processDeposit(rocketVault, rocketDAOProtocolSettingsDeposit);
}

In addition to the two checks implemented by poolCanDeposit, there's an extra condition that checks if deposits are enabled in the protocol settings contracts (rocketDAOProtocolSettingsDeposit.getDepositEnabled()). This condition should also be checked in poolCanDeposit as it will revert deposits in the derivative if deposits are not enabled in the protocol settings.

Proof of Concept

In the following test we simulate the conditions by impersonating the protocol's DAO account. First we make sure the maximum deposit settings can handle the current deposit value. Next we disable the deposits in the protocol settings (setSettingBool("deposit.enabled", false)) to replicate the described scenario. Then, when the user calls stake the deposit action will be routed to the Rocket Pool deposit function as poolCanDeposit will return true. The transaction will be reverted since the call to rocketDepositPool.deposit{value: msg.value}() will fail.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

// Run this test forking mainnet at block height 16906254
function test_Reth_deposit_MissingConditionInCanDeposit() public {
    // Setup derivative
    vm.prank(deployer);
    safEth.addDerivative(address(reth), 1e18);

    uint256 depositValue = 1 ether;

    RocketStorageInterface rocketStorage = RocketStorageInterface(reth.ROCKET_STORAGE_ADDRESS());
    address dao = rocketStorage.getAddress(keccak256(abi.encodePacked("contract.address", "rocketDAOProtocolProposals")));
    address daoSettings = rocketStorage.getAddress(keccak256(abi.encodePacked("contract.address", "rocketDAOProtocolSettingsDeposit")));

    uint256 currentMaximumDeposit = RocketDAOProtocolSettingsDepositInterface(daoSettings).getMaximumDepositPoolSize();

    // Ensure there's room to deposit
    vm.prank(dao);
    (bool success, ) = daoSettings.call(abi.encodeWithSignature("setSettingUint(string,uint256)", "deposit.pool.maximum", currentMaximumDeposit + depositValue));
    require(success);

    // DAO disables deposits
    vm.prank(dao);
    (success, ) = daoSettings.call(abi.encodeWithSignature("setSettingBool(string,bool)", "deposit.enabled", false));
    require(success);

    // Deal balance to user
    vm.deal(user, depositValue);

    // user tries to stake ether, transaction is reverted
    vm.prank(user);
    vm.expectRevert("Deposits into Rocket Pool are currently disabled");
    safEth.stake{value: depositValue}();
}

Recommendation

The poolCanDeposit function should also check that deposits are enabled in the protocol settings, and fallback to swapping if deposits are currently disabled.

    function poolCanDeposit(uint256 _amount) private view returns (bool) {
        address rocketDepositPoolAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked("contract.address", "rocketDepositPool")
                )
            );
        RocketDepositPoolInterface rocketDepositPool = RocketDepositPoolInterface(
                rocketDepositPoolAddress
            );

        address rocketProtocolSettingsAddress = RocketStorageInterface(
            ROCKET_STORAGE_ADDRESS
        ).getAddress(
                keccak256(
                    abi.encodePacked(
                        "contract.address",
                        "rocketDAOProtocolSettingsDeposit"
                    )
                )
            );
        RocketDAOProtocolSettingsDepositInterface rocketDAOProtocolSettingsDeposit = RocketDAOProtocolSettingsDepositInterface(
                rocketProtocolSettingsAddress
            );

        return
+           rocketDAOProtocolSettingsDeposit.getDepositEnabled() &&
            rocketDepositPool.getBalance() + _amount <=
            rocketDAOProtocolSettingsDeposit.getMaximumDepositPoolSize() &&
            _amount >= rocketDAOProtocolSettingsDeposit.getMinimumDeposit();
    }