Skip to content

Latest commit

 

History

History
94 lines (70 loc) · 3.75 KB

014.md

File metadata and controls

94 lines (70 loc) · 3.75 KB
Raw

Raspy Metal Rattlesnake

Medium

### Missing payable function in Unitroller.sol (Fallback Issue)

Description:

In the Unitroller.sol contract, the fallback function is defined as payable, meaning it can receive Ether. However, the contract does not seem to have a clear intention or need to accept Ether. This could result in unwanted behavior, such as receiving Ether from an untrusted source, which could cause issues in the system, such as locking funds or creating operational inconsistencies.

I have identified that the contract's fallback function is unnecessarily payable, and Ether could be sent without any validation. This might cause an attack vector if an attacker or user mistakenly sends funds to the contract.

Proof of Concept (PoC):

  1. Contract Issue
    The payable fallback function is defined in the contract as follows: https://github.com/sherlock-audit/2024-12-mach-finance/blob/main/contracts/src/Unitroller.sol#L136

    fallback() external payable {
    // delegate all other functions to current implementation
    (bool success,) = comptrollerImplementation.delegatecall(msg.data);
    assembly {
        let free_mem_ptr := mload(0x40)
        returndatacopy(free_mem_ptr, 0, returndatasize())

        switch success
        case 0 { revert(free_mem_ptr, returndatasize()) }
        default { return(free_mem_ptr, returndatasize()) }
    }
}

    Problem:

    • The contract is accepting Ether, but there’s no handling logic for it. This could lead to unexpected results, such as mistakenly locking Ether in the contract or causing unnecessary reverts.

  2. Test Case (Simulation): If a user or attacker sends Ether to the contract without any function call data (using .send() or .transfer()), the fallback function is triggered. This transaction could fail or cause unexpected outcomes.

    To simulate a test:

    function testEtherAcceptance() public {
    uint256 balanceBefore = address(unitroller).balance;
    try unitroller.send(1 ether) {
        assert(false, "Ether should not be accepted");
    } catch {
        // Expected revert, as the contract should not accept Ether
    }
}

    Expected Outcome:
    The contract should reject the Ether transfer or trigger a revert due to the payable fallback function, which isn't explicitly needed.

Recommendation:

  1. Remove the payable modifier from the fallback function if the contract is not intended to receive Ether.

    Proposed Code Change:

    fallback() external {
    // delegate all other functions to current implementation
    (bool success,) = comptrollerImplementation.delegatecall(msg.data);
    assembly {
        let free_mem_ptr := mload(0x40)
        returndatacopy(free_mem_ptr, 0, returndatasize())

        switch success
        case 0 { revert(free_mem_ptr, returndatasize()) }
        default { return(free_mem_ptr, returndatasize()) }
    }
}

  2. If Ether must be received, add a receive() function to control how Ether is handled in the contract, ensuring clarity and preventing unexpected issues.

    Proposed receive() function:

    receive() external payable {
    // Ether is accepted, but no action is taken
}

Impact:

  • Reward Manipulation: An attacker could send Ether to the contract without validation, causing issues in how funds are handled.
  • State Corruption: The fallback function could inadvertently lock Ether or disrupt contract execution.
  • Protocol Losses: If the contract is unexpectedly receiving Ether, funds could be locked or cause operational inconsistencies.