My solution for the Spearbit Writing Exercise.

Spearbit Technical Assessment.md

Delegatecall to a malicious contract

Severity: High

Context: Implementation.sol#L18

Since Implementation.sol is a contract without any access control implemented, anyone who has address for it can simply call it's functions. Hence it's possible for someone to make a delegate call directly from Implementation contract to a malicious contract which has function which has selfdestruct() functionality by directly calling the delegatecallContract(address a, bytes calldata _calldata) function of the Implementation contract, thus destorying Implementation contract and rendering the whole Wallet Protocol as useless. So an attacker:

1. Will create a malicious contract. A very basic one could be:

   contract Attack{
   constructor () {}
       function attack() {
           selfdestruct();
       }
   }
   

2. Will call delegatecallContract(address a, bytes calldata _calldata) function at the address of Implementation contract with parameters as address a = address of Attack contract and bytes calldata _calldata = function selector for "attack()".
3. Since delegatecall will run the code in context of the caller, attack() would be executed in context of Implementation and hence destoying it.

Recommendation: We can make "Imeplentation.sol" a library instead of a contract. Since libraries are stateless, it would be perfect to make "Imeplementation.sol" as a library as it must execute the code in caller's context without any scope for storage collision. Also, libraries cannot be destoryed. Since libraries cannot receive/hold ether, we must unmark both of the functions as "payable".

library Implementation { //modified one word -- "contract" --> "library"

    function callContract(address a, bytes calldata _calldata) external returns (bytes memory) { 
    // removed word "payable"
        (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) { 
    //removed word "payable"
        (bool success, bytes memory ret) =  a.delegatecall(_calldata);
        require(success);
        return ret;
    }