# Spearbit Writing Exercise Bug Report

# Author
**Name**: Parth Patel
**Email**: [e-mail]

# Findings

## delegatecall to a input-controlled function id

**Severity**: High

Context: [`Implementation.sol#L17-L21`] (https://github.com/spearbit-audits/writing-exercise/blob/develop/contracts/Implementation.sol#L17-L21)

As `Implementation` is deployed contract on its own, bad actor can call `delegatecallContract` method and delegate the execution to the malicious contract by passing arguments as address `a` of their malicious contract and the method they need to call in the malicious contract as `_calldata`. A malicious contract can then destructs `Implementation` contract. If `Implementation` contract gets destructed, all the Proxy contract freezes and can't send call to `Implementation` contract.

**Recommendation**:

Using `delegatecallContract` bad actors can delegate the calls to their contract, I recommend to **remove this method or making it callable only by trusted entities**. Both the options have pros and cons. If the `delegatecallContract` method is removed, there won't be any way for `Proxy` contract to change the `implementation` state variable. So, if this way is to be taken, the setter should be put into `Proxy` contract for setting `implementation` state variable. If the option of making it callable by only trusted entities is chosen, then `Implementation` contract needs to store the trusted entities in the contract. And condition needs to be added to check whether the `delegatecallContract` is called only by a trusted entity. Also, if `Implementation` contract needs to be changed, the whole state of the contract needs to be migrated to the new contract.

Another recommendation to mitigate this by just modifying `Implementation` contract is to use a modifier which only allows delegatecall and not the direct call to the function. Find the attached code below:

contract Implementation {

    address private immutable self = address(this);

    modifier onlyDelegateCall() {
        require(address(this) != self);
        _;
    }

    function callContract(address a, bytes calldata _calldata) payable external onlyDelegateCall returns (bytes memory) {
        (bool success , bytes memory ret) =  a.call{value: msg.value}(_calldata);
        require(success);
        return ret;
    }

    function delegatecallContract(address a, bytes calldata _calldata) payable external onlyDelegateCall returns (bytes memory) {
        (bool success, bytes memory ret) =  a.delegatecall(_calldata);
         require(success);
        return ret;
    }
}

**BONUS**: Since the contract Implementation is not intended to hold any state, it can be converted into `library`. The library doesn't have any state on blockchain and can't hold any ether. So, `payable` keyword from both the functions needs to be removed.

library Implementation {

    function callContract(address a, bytes calldata _calldata) external returns (bytes memory) {
        (bool success , bytes memory ret) =  a.call{value: msg.value}(_calldata);
        require(success);
        return ret;
    }

    function delegatecallContract(address a, bytes calldata _calldata) external returns (bytes memory) {
        (bool success, bytes memory ret) =  a.delegatecall(_calldata);
        require(success);
        return ret;
    }
}

## No zero-checks on address

**Severity**: Informational

Context: [`Proxy.sol#L18-L19`](https://github.com/spearbit-audits/writing-exercise/blob/develop/contracts/Proxy.sol#L18-L19)

There are no zero-checks for setting the addresses to the state variable.

**Recommendation**: Before setting the state variable(i.e `owner` and `implementation`) check whether the arguments provided to the constructor are non-zero.