Spearbit assignment solution.md

Spearbit-writing-exercise

Malicious user can brick all Proxy.sol instances resulting in all funds in them getting stuck

Severity: Critical

Context: Implementation.sol#L17

As stated in the README, every user will deploy its own Proxy.sol instance and every user will use the same shared Implementation.sol instance. Currently, Implementation.sol allows anyone to call its delegatecallContract method, which uses the delegatecall opcode to execute arbitrary code in the context of the Implementation.sol instance. A malicious user can call delegatecallContract method by passing it an address argument of the following smart contract's instance address:

// SPDX-License-Identifier: MIT
pragma solidity 0.8.10;

contract Attack {
    function die() external {
        selfdestruct(payable(address(0)));
    }
}

And then a _calldata argument that will call its die() method. This will result in the Implementation.sol instance calling the selfdestruct opcode, basically erasing its source code. When this happens, any call from the users' Proxy.sol instances to the shared Implementation.sol instance will do nothing, leaving all funds that the Proxy.sol instance owns stuck in it without an option for them to be withdrawn/moved.

Recommendation: Change Implementation.sol from a contract to a library. This way the code is still shared and reused, but libraries can't call the selfdestruct opcode, which solves the critical issue. Also, for the code to work you will have to remove the two payable keywords used. So solution is to replace contract with library keyword in Implementation.sol and also remove the two occurrences of payable in it.