Delegatecall to Destructed Contract in StakingProxy Fallback Function Leads to Unexpected Behavior

[c4-bot-3]

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingProxy.sol#L44

Vulnerability details

Impact

The use of delegatecall to a destructed or non-existent contract can lead to severe consequences and potential security vulnerabilities in the StakingProxy contract. If the implementation contract has been destructed, the proxy contract will still execute the delegatecall without any error, resulting in unexpected behavior and potential exploitation.

An attacker can intentionally destruct the implementation contract and then interact with the proxy contract, knowing that the delegatecall will still succeed. This can allow the attacker to bypass access control or validation checks in the proxy contract, execute unintended or malicious code, and potentially compromise the integrity and security of the system.

Moreover, without proper checks, the proxy contract may silently fail when delegating calls to a destructed implementation contract, making it difficult to detect and diagnose issues. This can lead to incorrect state updates, loss of funds, or other unintended consequences.

The issue violates the common assumption that a delegatecall to a non-existent or destructed contract will fail, which can introduce bugs and security risks that may be difficult to identify and mitigate.

Proof of Concept

https://github.com/code-423n4/2024-05-olas/blob/3ce502ec8b475885b90668e617f3983cea3ae29f/registries/contracts/staking/StakingProxy.sol#L44

fallback() external payable {
    assembly {
        let implementation := sload(SERVICE_STAKING_PROXY)
        calldatacopy(0, 0, calldatasize())
        let success := delegatecall(gas(), implementation, 0, calldatasize(), 0, 0)
        returndatacopy(0, 0, returndatasize())
        if eq(success, 0) {
            revert(0, returndatasize())
        }
        return(0, returndatasize())
    }
}

In this code, the delegatecall is executed without checking if the implementation contract exists. If the implementation contract has been destructed, the delegatecall will still return success, and the proxy contract will continue executing without any error.

Consider the following scenario:

1. The implementation contract is deployed and set as the implementation address in the StakingProxy contract.
2. Implementation contract no longer exists for some reason or is destructed.
3. The attacker interacts with the StakingProxy contract, triggering the fallback() function.
4. The delegatecall to the destructed implementation contract still succeeds, and the proxy contract continues executing without any error.

Tools Used

• Manual

Recommended Mitigation Steps

Add a check to ensure that the implementation contract exists before executing the delegatecall. Here's the modified fallback() function with the necessary check:

fallback() external payable {
    address implementation;
    assembly {
        implementation := sload(SERVICE_STAKING_PROXY)
    }

    // Check if the implementation contract exists
    require(implementation.code.length > 0, "Implementation contract does not exist");

    assembly {
        calldatacopy(0, 0, calldatasize())
        let success := delegatecall(gas(), implementation, 0, calldatasize(), 0, 0)
        let returnDataSize := returndatasize()

        // Check the return data size before copying to memory
        if gt(returnDataSize, 0xffffffffffffffff) {
            revert(0, 0)
        }

        returndatacopy(0, 0, returnDataSize)

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

Assessed type

call/delegatecall