Summary

In the fallback function of the Proxy contract, there is an assembly block that is responsible for delegating the call to the implementation contract. However, there is an issue with the way the result of the delegate call is being handled. Specifically, the contract is currently using a switch statement to handle the delegatecall result, and while it handles the success case, it incorrectly handles the failure case.

Vulnerability Details

The issue lies in the following section of the code:

switch result
case 0 { revert(ptr, size) }
default { return(ptr, size) }
}

In the context of a delegatecall, a result of 0 indicates a failure, and a non-zero result indicates success. In the code above, the case 0 block correctly reverts the transaction on failure, but the default block is used to return the data on success. This is incorrect because the value of result can be any non-zero value on success, not just 1.

Impact

The incorrect handling of the delegate call result could lead to unexpected behavior and vulnerabilities in the smart contract system. If a delegate call fails, the intended behavior is to revert the transaction, but due to the flawed code, the transaction may not be reverted as expected. This could potentially lead to unauthorized or unintended execution of functions and manipulation of the contract's state.

PoC

1 . Deploy the Implementation contract:

// Implementation contract
pragma solidity ^0.8.0;

contract Implementation {
    function doSomething() external {
        require(false, "Intentional require failure");
    }
}

2 . Deploy the Implementation contract.
3 . Deploy the Proxy contract, providing the address of the deployed Implementation contract as the implementation address.
Call the doSomething function on the proxy contract.

In this scenario, the doSomething function in the Implementation contract contains a require statement that will always fail. When the Proxy contract delegates the call to the Implementation contract's doSomething function, the delegate call will fail due to the require statement.

• Expected Behavior :
  The transaction should revert, indicating a failure due to the requirement in the doSomething function of the Implementation contract.

• Actual Behavior :
  Due to the bug in the proxy contract's fallback function, the transaction might not revert as expected, and unintended behavior or state changes may occur. The delegate call's failure is not properly handled, potentially leading to vulnerabilities.

Tools Used

Manual Review

Recommendations

we should explicitly check for a non-zero value in the result variable to determine if the delegate call was successful. Here's the corrected code:

fallback() external {
    address implementation = _implementation;
    assembly {
        let ptr := mload(0x40)
        calldatacopy(ptr, 0, calldatasize())
        let result := delegatecall(gas(), implementation, ptr, calldatasize(), 0, 0)
        let size := returndatasize()
        returndatacopy(ptr, 0, size)

        if eq(result, 0) {
            revert(ptr, size)
        }
        return(ptr, size)
    }
}