code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-livepeer/blob/main/contracts/bonding/BondingManager.sol#L285 https://github.com/code-423n4/2023-08-livepeer/blob/main/contracts/token/Minter.sol#L192

Vulnerability details

Vulnerability Details

The BondingManager's withdrawFees function transfers the fees to the caller of this function.

function withdrawFees(
    address payable _recipient,
    uint256 _amount
) external whenSystemNotPaused currentRoundInitialized autoClaimEarnings(msg.sender) {
    require(_recipient != address(0), "invalid recipient");

    uint256 fees = delegators[msg.sender].fees;

    require(fees >= _amount, "insufficient fees to withdraw");

    delegators[msg.sender].fees = fees.sub(_amount);

    // Tell Minter to transfer fees (ETH) to the address
    minter().trustedWithdrawETH(_recipient, _amount);

    emit WithdrawFees(msg.sender, _recipient, _amount);
}

BondingManager.sol - Line 285

This function uses the trustedWithdrawETH function of Minter contract which uses the transfer function to send the funds to _recipient.

function trustedWithdrawETH(
    address payable _to,
    uint256 _amount
) external onlyBondingManagerOrJobsManager whenSystemNotPaused {
    _to.transfer(_amount);
}

Minter.sol - Line 192

Impact

transfer() uses a fixed amount of gas (2300), which was used to prevent reentrancy. However this limit the protocol that need more gas than that to process the transaction.

Tools Used

Manual Review

Recommended Mitigation Steps

Use call instead of transfer.

function trustedWithdrawETH(
    address payable _to,
    uint256 _amount
) external onlyBondingManagerOrJobsManager whenSystemNotPaused {
-   _to.transfer(_amount);
+   (bool success, ) = _to.call{value: _amount}("");
+   require(success, "Minter: withdraw ETH failed.");
}

Assessed type

ETH-Transfer

c4-pre-sort commented 1 year ago

141345 marked the issue as duplicate of #129

c4-judge commented 1 year ago

HickupHH3 marked the issue as unsatisfactory: Out of scope

alymurtazamemon commented 1 year ago

Hello @HickupHH3,

I respect your judgment over this issue, yes we know the transfer function is inside the Minter contract which is out of scope but here we are talking about the affected area which is in scope.

If the user claims their earnings and their transaction failed due to the transfer function and they face the DOS issue then the point of failure will be considered in the BoindingManager's withdrawFees function, not in the Minter contract's function.

Here I want to present some evidence for this issue:

1. Confirmation from Sponsor

I especially for this issue confirmed by the sponsor before writing the script.

2. Some Evidence from the Past Contests (Code4rena + Sherlock)

There are tons of evidence available on Solodit, if you type out of scope in the keyword field and search. Here I am mentioning some which help with this issue.

These all were pointing to the issue in the contracts which were in scope but were affected by the code which was out of scope.

HickupHH3 commented 1 year ago

the protocol is not meant to be used with contracts as transcoders or delegators, only EOAs

my judgement was based off this expectation. It's QA at best, as a recommendation.

alymurtazamemon commented 1 year ago

@HickupHH3 Yes but they have mentioned not meant to be and there is no any check that validation the caller should not be a contract.

Also if you know, using higher than 2300 gas might be mandatory for some multisig wallets.

I must say you should check this Cyfrin Issue - Reported by Hans and 0kage.

Thanks