25921 - [SC - Insight] Flaw in upgradeToAndCall leads to the proxy cal....md

Flaw in upgradeToAndCall leads to the proxy calling himself, which sets msg.sender to the proxy instead of Timelock

Submitted on Nov 21st 2023 at 03:11:43 UTC by @neth for Boost | DeGate

Report ID: #25921

Report type: Smart Contract

Report severity: Insight

Target: https://etherscan.io/address/0x54D7aE423Edb07282645e740C046B9373970a168#code

Impacts:

• Contract fails to deliver promised returns, but doesn't lose value

Description

Bug Description

In OwnedUpgradeabilityProxy, function upgradeToAndCall

  function upgradeToAndCall(address implementation, bytes memory data) payable public onlyProxyOwner {
    upgradeTo(implementation);
    (bool success, ) = address(this).call{value: msg.value}(data);
    require(success);
  }

calls himself by doing address(this).call..., which redirects to the fallback function in Proxy with msg.sender == address(this), instead of directly delegatecalling to the implementation with the caller being Timelock, which is bad as proxies are not supposed to "initiate" transactions, but delegate them.

Impact

In the implementation, the owner is responsible for calling setCheckBalance, which is, at the moment, an EOA (see https://etherscan.io/address/0xacD3A62F3eED1BfE4fF0eC8240d645c1F5477F82) but it seems it will be Timelock in the future. That means it is NOT, by any means, the proxy, so trying to update the implementation via upgradeToAndCall will revert, as the modifier will check for msg.sender == owner with owner != address(proxy).

The same applies to ExchangeProxy

Recommendation

  function upgradeToAndCall(address implementation, bytes memory data) payable public onlyProxyOwner {
    upgradeTo(implementation);
-   (bool success, ) = address(this).call{value: msg.value}(data);
+   _fallback();
    require(success);
  }

Proof of concept

(Simplified POC, if you need the mainnet fork I can give it to you but it was far more verbose)

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

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

contract Proxy {
    fallback() payable external {
        console2.log("Caller in Proxy::fallback, which is gonna be passed to impl (shall be Timelock)");
        console2.log(msg.sender);
        console2.log("");
        console2.log("It was the address of the Proxy, see the next log which outputs address(this)");
        console2.log(address(this));
    }

    function updateToAndCall() external {
        console2.log("Caller in Proxy::updateToAndCall, that is, Timelock");
        console2.log(msg.sender);
        console2.log("");
        address(this).call("");
    } 
}

contract POC is Test {

    Proxy internal proxy;

    function setUp() external {
        proxy = new Proxy();
    }

    function testPOC() external {
        console2.log("Address of the initial caller, that is, Timelock");
        console2.log(address(this));
        console2.log("");
        proxy.updateToAndCall();
    } 
}