The Graph L2 bridge contest - csanuragjain's results

Lines of code

https://github.com/code-423n4/2022-10-thegraph/blob/main/contracts/gateway/BridgeEscrow.sol#L21

Vulnerability details

Impact

The implementation contract can initialize the _controller address multiple times on Managed contract even though it should only be allowed once. Ideally only existing controller should be allowed to decide the new controller but this can be used to bypass the controller power.

Changing controller should only be allowed via setController function

Proof of Concept

1. Observe the initialize function

function initialize(address _controller) external onlyImpl {
        Managed._initialize(_controller);
    }

2. As we can see the Implementation contract can call this multiple time in order to change the controller via Managed._initialize(_controller);

function _initialize(address _controller) internal {
        _setController(_controller);
    }

function _setController(address _controller) internal {
        require(_controller != address(0), "Controller must be set");
        controller = IController(_controller);
        emit SetController(_controller);
    }

3. As we can see that this is giving the power to implementation contract to change the controller using _setController function when ideally _setController should only be allowed to be called through current controller via setController function. This could be used to bypass current controller and set a new controller without consent from existing controller

function setController(address _controller) external onlyController {
        _setController(_controller);
    }

Recommended Mitigation Steps

Use the initializer modifier on the initialize function making it callable once only