# Spearbit writing exercise
# devtooligan

# Introduction

### [Spearbit Writing Exercise Wallet Protocol](https://github.com/spearbit-audits/writing-exercise)
You are given an implementation for a smart contract wallet. There are two contracts

1. [`Implementation.sol`](https://github.com/spearbit-audits/writing-exercise/blob/de45a4c5b654710812e9fa29dde6e12526fe4786/contracts/Implementation.sol#L17): Deployed once and used as implementation contract in `Proxy.sol`.
2. [`Proxy.sol`](https://github.com/spearbit-audits/writing-exercise/blob/de45a4c5b654710812e9fa29dde6e12526fe4786/contracts/Proxy.sol): Each user has a unique `Proxy` deployment with the above implementation. This is a simply proxy contract which delegatecalls the implementation contract. It has an access control check that allows only the owner to use the fallback function.

The idea is that users can keep their funds, for example, ETH or ERC20 tokens in the Proxy. To use these funds, users can execute arbitrary calls and arbitrary delegatecalls by using the implementation contract (it has `callContract` and `delegatecallContract`). The implementation contract is deployed only once and reused to save gas.

There is a **critical bug** in the wallet protocol. The exercise is to find it and write it in markdown format, in accordance with the style guide.

# Findings

## `delegatecallContract` is a Pandora's box of vulnerabilities.

**Severity**: High

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

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

There are multiple vulnerabilities introduced by performing a `delegatecall` within this `Implementation.sol` function. Each of these is addressed below in more detail:
- [Anyone can call this function and `selfdestruct` the`Implementation.sol` contract at any time.](#Anyone-can-selfdestruct-the-implementation-contract)
- [Through accident or interaction with a malicious contract, the `implementation` storage slot can be overwritten in `Proxy.sol`.](#The-implementation-address-stored-in-Proxysol-can-be-overwritten)
- [General complexity around the context used for `delegatecall` is a major footgun that could result in a multitude of undesirable effects.](https://hackmd.io/FGB1oRMDTT2RwZTzL7vwpg#General-complexity-and-risk-around-delegatecallContract)
- [It can be inadvertantly called on an address with no code without any indication of failure.](#Calls-can-be-made-to-addresses-without-code)

**Recommendation**:

--- a/writing-exercise/Implementation.sol
+++ b/writing-exercise/Implementation.sol
@@ -13,10 +13,4 @@ contract Implementation {
-
-    function delegatecallContract(address a, bytes calldata _calldata) payable external returns (bytes memory) {
-        (bool success, bytes memory ret) =  a.delegatecall(_calldata);
-        require(success);
-        return ret;
-    }
 }

**We recommend removing the `delegatecallContract` function from `Implementation.sol`.** This is consistent with [OpenZeppelin's guidance](https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#potentially-unsafe-operations) when using the proxy upgradeable pattern:
> it is not allowed to use either selfdestruct or delegatecall

Furthermore, we were unable to find any examples of similar `delegatecallContract` type functionality in other wallet protocols (in the past 3 years), so this change is keeping with current industry standards. If there is specific functionality required by `Implementation.sol`, then we recommend incorporating it directly into the contract. Because of its upgradeable design, functionality can be added/removed in the future as needed.

---

## Anyone can `selfdestruct` the `Implementation` contract.

**Severity**: High

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

Because the `Implementation` contract's functions are not restricted, someone could call the `delegatecallContract` function and pass in the address and data for a function that invokes `selfdestruct`. Since `delegatecall` uses the context of the calling contract, in this case the `Implementation` contract would be destroyed.

With the `Implementation` destroyed, all of the `Proxy` contracts would now be rendered useless. A new `Implementation` contract would have to be deployed. Since there is no setter for the `implementation` variable in `Proxy.sol`, all of these contracts would also have to be redeployed with updated `Implementation` contract addresses.

Borrowing a diagram from a [Trail of Bits write-up](https://blog.trailofbits.com/2020/12/16/breaking-aave-upgradeability/) of a similar vulnerability found in Aave, here is a simplified visual representation of the exploit:


Proof of concept:

contract Attacker {

    function boom(address payable to) public {
        selfdestruct(to);
    }

    // Attacker calls this function passing in the Implementation address.
    function attack(address impl) public {
        bytes memory data = abi.encodeWithSelector(this.boom, payable(address(this)));
        Implementation(impl).delegatecallContract(address(this), data);
    }
}

**Recommendation**: [As mentioned above](#delegatecallContract-is-a-Pandora%E2%80%99s-box-of-vulnerabilities), our recommendation is to remove the `delegatecallContract` function altogether. Alternative actions to remedy this vulnerability include:

1. **Check address(this) is not the address of the `Implementation` contract.**

--- a/writing-exercise/contracts/Implementation.sol
+++ b/writing-exercise/contracts/Implementation.sol
@@ -8,6 +8,12 contract Implementation {

+    address public immutable addressThis;
+
+    constructor() {
+        addressThis = address(this);
+    }
+

@@ -15,6 +21,7

     function delegatecallContract(address a, bytes calldata _calldata) payable external returns (bytes memory) {
+        require(address(this) != addressThis, "delegatecall only");

During a `delegatecall` the function uses the context of the calling contract, including the address. This check would prevent the function from being called directly.

**NOTE:** This would not prevent the `Proxy` contract from selfdestruct in the event the proxy `owner` calls a malicious contract using `delegatecallContract` through the proxy.

2. **Change `Implementation.sol` to a library.**

--- a/writing-exercise/contracts/Implementation.sol
+++ b/writing-exercise/contracts/Implementation.sol
@@ -6,19 +6,15
 /// The implementation contract for the Proxy (see: `Proxy.sol`).
-contract Implementation {
+library Implementation {

-    function callContract(address a, bytes calldata _calldata) payable external returns (bytes memory) {
+    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) payable external returns (bytes memory) {
+    function delegatecallContract(address a, bytes calldata _calldata) external returns (bytes memory) {

Since external libraries are called via delegatecall, the `Implementation` contract function cannot be called directly by an outsider.

**NOTE:** This would not prevent the `Proxy` contract from `selfdestruct` in the event the proxy owner calls a malicious contract using `delegatecallContract` through the proxy.

3. **Use `create2` to deploy the `Implementation` contract.**

This would be the least amount of mitigation and is not recommended as the only remedy. Using `create2`, if the `Implmentation` contract were destroyed, then the same contract could be redeployed to the same address with all of the `Proxy` contracts still pointing to the correct address. As an aside, `create2` can also be used with the `Proxy` contracts.

---

## The `implementation` address stored in `Proxy.sol` can be overwritten.

**Severity**: High

Context: [`Proxy.sol#L15`](https://github.com/spearbit-audits/writing-exercise/blob/de45a4c5b654710812e9fa29dde6e12526fe4786/contracts/Proxy.sol#L15), [`Implementation.sol#L17-L20`](https://github.com/spearbit-audits/writing-exercise/blob/de45a4c5b654710812e9fa29dde6e12526fe4786/contracts/Implementation.sol#L17-L20)

Either by mistake, or as a result of a bait and switch, with `delegatecallContract` the proxy owner can overwrite the storage slot for the `implementation` state variable. At best, this would result in an inoperable proxy. At worst, the `Implementation` contract can be replaced with a malicious contract that could take funds or commit other nefarious activities.

For example, the proxy owner calls `delegatecallContract` on the `seemsHarmless` function of the following contract resulting in the `Implementation` contract being replaced by a malicious contract.

contract BaitAndSwitch {
    address constant maliciousImplementationContract = address(0xBADC0DE);

    // This is the same storage slot (0) as the `implementation` variable in the Proxy
    address public slot0;

    function seemsHarmless() public {
        slot0 = maliciousImplementationContract;
    }
}

**Recommendation**: [As mentioned above](#delegatecallContract-is-a-Pandora%E2%80%99s-box-of-vulnerabilities), our recommendation is to remove the `delegatecallContract` function altogether.

One alternative action to remedy this vulnerability is to store the `implementation` variable somewhere other than slot 0. This is one of the features of [EIP-1967](https://eips.ethereum.org/EIPS/eip-1967):

>To avoid clashes in storage usage between the proxy and logic contract, the address of the logic contract is typically saved in a specific storage slot

--- a/writing-exercise/contracts/Proxy.sol
+++ b/writing-exercise/contracts/Proxy.sol
@@ -12,13 +12,27 @@ pragma solidity 0.8.10;
 /// The implementation will be set to a deployment of `Implementation.sol`.
 contract Proxy {
     address immutable public owner;
-    address public implementation;
+
+    // keccak256("org.spearbit.mafia.proxy.implementation");
+    bytes32 internal constant IMPLEMENTATION_SLOT =
+        0x3e6bd77e4c941aba444686825fcdbbef7ad9448a393f4908a91c8bab921d84cd;

     constructor(address _implementation, address _owner) {
         owner = _owner;
-        implementation = _implementation;
+        bytes32 slot = IMPLEMENTATION_SLOT;
+        assembly {
+            sstore(slot, _implementation)
+        }
+    }
+
+    function implementation() internal view returns (address impl) {
+        bytes32 slot = IMPLEMENTATION_SLOT;
+            assembly {
+            impl := sload(slot)
+        }

**NOTE:** This would only mitigate the chance of accidentally overwriting the `implementation` slot. A bad actor could easily determine the slot and create a malicious contract to overwrite it.

---

## General complexity and risk around `delegatecallContract`.

**Severity**: High

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

As recently as 23-July-2022, it was reported that [Audius was compromised](https://blog.openzeppelin.com/audius-contracts-audit/) through an exploit of the Open Zeppelin proxy upgradeable pattern which used `delegatecall`. Interestingly, the bug had existed in the code for over 2 years, and passed undetected through an [audit by Open Zeppelin themselves in 2020](https://blog.openzeppelin.com/audius-contracts-audit/).

The heart of the complexity stems from a general lack of understanding of how `delegatecall` works and the potentially disastrous outcomes certain actions could have. These outcomes could include draining of Ether, token, and NFT balances.

**Recommendation**: [As mentioned above](#delegatecallContract-is-a-Pandora%E2%80%99s-box-of-vulnerabilities), our recommendation is to remove the `delegatecallContract` function altogether.

One alternative action to remedy this vulnerability is to create a registry to whitelist both the contract addresses and the signatures of approved functions.

Example:

--- a/writing-exercise/contracts/Implementation.sol
+++ b/writing-exercise/contracts/Implementation.sol
@@ -7,6 +7,13 @@ contract Implementation {
+    mapping(address => mapping(bytes4 => bool)) public registeredFunctions;
+
+    function registerFunction(address contractAddress, bytes4 funcSig) external {
+        require(msg.sender == owner);
+        registeredFunctions[contractAddress][funcSig] = true;
+    }

@@ -15,6 +22,7 @@

     function delegatecallContract(address a, bytes calldata _calldata) payable external returns (bytes memory) {
+        require(registeredFunctions[a][bytes4(_calldata)], "unregistered function");

**Note:** This is just a proof of concept. A full implementation would include a way to unregister and would also ensure the storage slot did not collide with `Proxy`. Also, this remedy offers no guarantee that the owner will not inadvertantly register a malicious function.

---

## Calls can be made to addresses without code.

**Severity**: Medium

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

Neither `delegatecallContract` nor `callContract` perform checks for the existence of code at the target address. If a wrong address is provided, then, at best, gas will be wasted and, at worst, funds could be sent to an irretrievable address.

**Recommendation**:

--- a/writing-exercise/contracts/Implementation.sol
+++ b/writing-exercise/contracts/Implementation.sol
@@ -2,19 +2,21 @@
 contract Implementation {

     function callContract(address a, bytes calldata _calldata) payable external returns (bytes memory) {
+        require(a.code.length > 0, "no code");
         (bool success , bytes memory ret) =  a.call{value: msg.value}(_calldata);
         require(success);
         return ret;
     }

     function delegatecallContract(address a, bytes calldata _calldata) payable external returns (bytes memory) {
+        require(a.code.length > 0, "no code");

This would prevent accidentally calling `callContract` or `delegatecallContract` on an address that does not contain code.

**Note:** As an aside, the opposite check,`code.length == 0`, should never be considered a guarantee that the address is an EOA. If called from the constructor, this will return 0 as would a create2 address with nothing yet deployed.

---

## No setter on the `implementation` address in `Proxy`

**Severity**: Medium

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

The `implementation` contract address is stored in a state variable in `Proxy.sol` and set in the constructor. However, there is no way to change it after that. If a bug is subsequently found in the `Implementation` contract or if upgradeability is desired, it may not be possible to change it.

**Recommendation**:

--- a/writing-exercise/contracts/Proxy.sol
+++ b/writing-exercise/contracts/Proxy.sol
@@ -19,6 +19,11 @@ contract Proxy {

+    function setImplementation(address implementation_) external {
+        require(msg.sender == owner, "unauthorized");
+        implementation = implementation_;
+    }
+

**Note:** If the `Implementation` contract is in tact with the `delegatecallContract` function still operating, then a workaround to this would be to call a contract with a function that would update the storage slot accordingly. However, not only is that complex and prone to irrecoverable mistakes, but also it would not work if, for example, the `Implementation` contract were to `selfdestruct` or had another bug preventing this course of action.

**Also note:** If it is intended that `Proxy.sol` only ever have one `implementation` which can never be changed, then this should be changed to an `immutable` which would eliminate the chance of storage slot collision and also save significant runtime gas.

---

## Optimize byte code for `Proxy.sol`

**Severity**: Gas optimization

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

Because `Proxy.sol` is a straightforward shim contract that will potentially be called many times, we suggest optimizing at the byte code level for the least amount of runtime gas usage. The shim bytecode can then be used by a factory to `create` (or `create2`) proxies as needed. Here is an example of optimized bytecode to replace `Proxy.sol`:

// Contract creation
[00]    RETURNDATASIZE  // 0x00
[01]    PUSH1    20
[03]    DUP1
[04]    PUSH1    0a
[06]    RETURNDATASIZE
[07]    CODECOPY
[08]    DUP2
[09]    RETURN

// Proxy.sol code begins

// onlyOwner
[0a]    CALLER         [msg.sender]
[0b]    PUSH20 <owner> [owner, msg.sender] 
[0c]    EQ             [owner == msg.sender] 
[0d]    PUSH1 0x12     [0x12, owner == msg.sender]
[0e]    JUMPI          []
[0f]    RETURNDATASIZE [0] // use RETURNDATASIZE for 0x0 to save gas
[10]    RETURNDATASIZE [0, 0]
[11]    REVERT         // UNAUTHORIZED

// Copy calldata to memory for the DELEGATECALL
[12]    JUMPDEST
[13]    CALLDATASIZE   [size]
[14]    RETURNDATASIZE [0,size]
[15]    RETURNDATASIZE [0,0,size]
[16]    CALLDATACOPY   []

// Prepare stack for DELEGATECALL
[17]    RETURNDATASIZE  [0]
[18]    RETURNDATASIZE  [0,0]
[19]    RETURNDATASIZE  [0,0,0]
[1a]    CALLDATASIZE    [size, 0, 0, 0]
[1b]    RETURNDATASIZE  [0, size, 0, 0, 0]
[1c]    PUSH20<target>  [<target address>, 0, size, 0, 0, 0]
[1d]    GAS             [gas, <target address>, 0, size, 0, 0, 0]

// Execute DELEGATECALL
[1e]    DELEGATECALL    [1, 0]
[1f]    RETURNDATASIZE  [0,1,0]
[20]    DUP3            [0,0,1,0]
[21]    DUP1            [0,0,0,1,0]
[22]    RETURNDATACOPY  [1,0]
[23]    SWAP1           [0,1]
[24]    RETURNDATASIZE  [0,0,1]
[25]    SWAP2           [1,0,0]
[26]    PUSH1 0x29      [3a,1,0,0]
[27]    JUMPI           [0,0]
[28]    REVERT
[29]    JUMPDEST
[2a]    RETURN