[Audit Report] PoolTogether

0xDave·2023년 1월 24일

Ethereum

목록 보기

91/112

주요 취약점

[M-01] AN ATTACKER CAN MAKE USERS UNABLE TO CANCEL THEIR L1 CALLS ON ETHEREUM TO ARBITRUM

일단 취약점을 보기 전에 이더리움(L1)에서 아비트럼(L2)으로 어떻게 메세지를 전달하는지 알아보자. 아비트럼 docs에 따르면 Retryable ticket이라는 것을 만들어서 L2에 전달한다. 이 때 2가지 step으로 진행된다.

PoolTogether 코드에서는 먼저 relayCalls를 호출해 논스값과 함께 전달할 메세지를 모으고 이후 processCalls를 호출해서 L2로 메세지를 전달한다. 이 때 relayCalls는 Relayer 코드 안에서 동작하는데 Relayer라는 개념이 명확히 잡히지 않아서 찾아봤다.

해당 취약점이 문제가 되는 부분은 processCalls 함수에 있다. 앞서 설명했듯이 inbox.createRetryableTicket을 통해 티켓을 만든다.

  function processCalls(
    uint256 _nonce,
    CallLib.Call[] calldata _calls,
    address _sender,
    uint256 _gasLimit,
    uint256 _maxSubmissionCost,
    uint256 _gasPriceBid
  ) external payable returns (uint256) {
    require(relayed[_getTxHash(_nonce, _calls, _sender, _gasLimit)], "Relayer/calls-not-relayed");

    bytes memory _data = abi.encodeWithSignature(
      "executeCalls(uint256,address,(address,bytes)[])",
      _nonce,
      _sender,
      _calls
    );

    uint256 _ticketID = inbox.createRetryableTicket{ value: msg.value }(
      address(executor),
      0,
      _maxSubmissionCost,
      msg.sender,
      msg.sender,
      _gasLimit,
      _gasPriceBid,
      _data
    );

    emit ProcessedCalls(_nonce, msg.sender, _ticketID);

    return _ticketID;
  }

createRetryableTicket 함수를 자세히 살펴보자.

    function createRetryableTicket(
        address to,
        uint256 l2CallValue,
        uint256 maxSubmissionCost,
        address excessFeeRefundAddress,
        address callValueRefundAddress,
        uint256 gasLimit,
        uint256 maxFeePerGas,
        bytes calldata data
    ) external payable whenNotPaused onlyAllowed returns (uint256) {
        // ensure the user's deposit alone will make submission succeed
        if (msg.value < (maxSubmissionCost + l2CallValue + gasLimit * maxFeePerGas)) {
            revert InsufficientValue(
                maxSubmissionCost + l2CallValue + gasLimit * maxFeePerGas,
                msg.value
            );
        }

        // if a refund address is a contract, we apply the alias to it
        // so that it can access its funds on the L2
        // since the beneficiary and other refund addresses don't get rewritten by arb-os
        if (AddressUpgradeable.isContract(excessFeeRefundAddress)) {
            excessFeeRefundAddress = AddressAliasHelper.applyL1ToL2Alias(excessFeeRefundAddress);
        }
        if (AddressUpgradeable.isContract(callValueRefundAddress)) {
            // this is the beneficiary. be careful since this is the address that can cancel the retryable in the L2
            callValueRefundAddress = AddressAliasHelper.applyL1ToL2Alias(callValueRefundAddress);
        }

        return
            unsafeCreateRetryableTicket(
                to,
                l2CallValue,
                maxSubmissionCost,
                excessFeeRefundAddress,
                callValueRefundAddress,
                gasLimit,
                maxFeePerGas,
                data
            );
    }

중간에 특이한 주석이 있다. callValueRefundAddress 주소가 retryable을 취소할 수 있다는 내용이다.

// this is the beneficiary. be careful since this is the address that can cancel the retryable in the L2

해당 주소는 createRetryableTicket 함수의 5번 째 파라미터에 들어가는 주소로 processCalls 함수 내부에서는 msg.sender로 정의되어 있다. 즉, processCalls 함수를 호출하는 주소가 retryable을 취소할 수 있다는 뜻이다.

여기서 문제가 발생한다. 다른 사람이 모아놓은 메세지를 processCalls 함수를 호출했던 악의적인 공격자가 언제든지 취소시킬 수 있는 상황이 발생한다. 꼭 악의적이지 않더라도 이런 상황도 생각해 볼 수 있다.

A가 relayCalls를 호출해서 메세지를 모았다.
B가 processCalls 함수를 호출해서 메세지를 전달한다.
A는 자신이 모은 메세지에 포함된 자신의 트랜잭션을 취소하고 싶다.
하지만 취소 권한은 B에게 있기 때문에 트랜잭션을 취소할 수 없다.

따라서 취소 권한이 원래 트랜잭션을 보낸 사람에게 종속되도록 수정해야 한다. createRetryableTicket()에 전달하는 파라미터를 msg.sender가 아닌 _sender로 변경하는 것이 좋다.

[M-02] WHEN A SMART CONTRACT CALLS CROSSCHAINRELAYERARBITRUM.PROCESSCALLS, EXCESS SUBMISSION FEES MAY BE LOST

     * @notice Put a message in the L2 inbox that can be reexecuted for some fixed amount of time if it reverts
     * @dev all msg.value will deposited to callValueRefundAddress on L2
     * @param destAddr destination L2 contract address
     * @param l2CallValue call value for retryable L2 message
     * @param  maxSubmissionCost Max gas deducted from user's L2 balance to cover base submission fee
     * @param excessFeeRefundAddress maxgas x gasprice - execution cost gets credited here on L2 balance
     * @param callValueRefundAddress l2Callvalue gets credited here on L2 if retryable txn times out or gets cancelled
     * @param maxGas Max gas deducted from user's L2 balance to cover L2 execution
     * @param gasPriceBid price bid for L2 execution
     * @param data ABI encoded data of L2 message
     * @return unique id for retryable transaction (keccak256(requestID, uint(0) )
     */
    function createRetryableTicket(
        address destAddr,
        uint256 l2CallValue,
        uint256 maxSubmissionCost,
        address excessFeeRefundAddress,
        address callValueRefundAddress,
        uint256 maxGas,
        uint256 gasPriceBid,
        bytes calldata data
    ) external payable virtual override onlyWhitelisted returns (uint256) {
        // ensure the user's deposit alone will make submission succeed
        require(msg.value >= maxSubmissionCost + l2CallValue, "insufficient value");

        // if a refund address is a contract, we apply the alias to it
        // so that it can access its funds on the L2
        // since the beneficiary and other refund addresses don't get rewritten by arb-os
        if (shouldRewriteSender && Address.isContract(excessFeeRefundAddress)) {
            excessFeeRefundAddress = AddressAliasHelper.applyL1ToL2Alias(excessFeeRefundAddress);
        }
        if (shouldRewriteSender && Address.isContract(callValueRefundAddress)) {
            // this is the beneficiary. be careful since this is the address that can cancel the retryable in the L2
            callValueRefundAddress = AddressAliasHelper.applyL1ToL2Alias(callValueRefundAddress);
        }

        return
            createRetryableTicketNoRefundAliasRewrite(
                destAddr,
                l2CallValue,
                maxSubmissionCost,
                excessFeeRefundAddress,
                callValueRefundAddress,
                maxGas,
                gasPriceBid,
                data
            );
    }

excessFeeRefundAddress에 대한 주석에 초과되는 실행 비용은 L2에 속한다라고 되어있다. 여기까지는 딱히 문제 될 것이 없다.

* @param excessFeeRefundAddress maxgas x gasprice - execution cost gets credited here on L2 balance

그런데 createRetryableTicket을 호출하는 주체가 EOA가 아니라 컨트랙트라면 얘기가 달라진다. 보통 L1을 사용하든 L2를 사용하든 EOA의 주소는 그대로 유지된다. 사용하는 네트워크가 달라지는 것이기 때문에 다른 L1을 사용하는 것과 동일하게 생각하면 된다. 하지만 컨트랙트는 다르다. 이더리움에서 만든 컨트랙트가 아발란체에도 같은 주소로 존재하지 않는 것처럼 L2인 아비트럼에도 존재하지 않을 것이다. 따라서 컨트랙트가 초과되는 비용을 지불하고 함수를 호출했을 경우엔 그 비용을 되찾을 수 없다.

    uint256 _ticketID = inbox.createRetryableTicket{ value: msg.value }(
      address(executor),
      0,
      _maxSubmissionCost,
      msg.sender,
      msg.sender,
      _gasLimit,
      _gasPriceBid,
      _data
    );

이를 방지하기 위해선 processCalls 내부에서 createRetryableTicket를 호출할 때 들어가는 파라미터를 msg.sender가 아닌 사용자가 입력하거나 특정할 수 있게 수정해야 한다.

3. [M-03] CROSSCHAINEXECUTOR CONTRACTS DO NOT UPDATE THE NECESSARY STATES FOR FAILING TRANSACTIONS

  function executeCalls(
    uint256 _nonce,
    address _sender,
    CallLib.Call[] calldata _calls
  ) external {
    ICrossChainRelayer _relayer = relayer;
    _isAuthorized(_relayer);

    bool _executedNonce = executed[_nonce];
    executed[_nonce] = true;

    CallLib.executeCalls(_nonce, _sender, _calls, _executedNonce);

    emit ExecutedCalls(_relayer, _nonce);
  }

executeCalls 함수 내부에서 CallLib.executeCalls을 통해 트랜잭션을 실행한다. 그런데 보통 success 여부를 체크하는 일반적인 call과 다르게 이러한 체크여부가 없다. 여러 트랜잭션을 한 번에 실행할텐데 중간에 하나가 실패해도 모든 트랜잭션이 revert 날 것이다. 추가적으로 중간에 어떤 트랜잭션이 실패했는지 알 수 없고, 어디서 실패했는지 모르기 때문에 계속 시도하다가 gas만 소모할 것이다.(모든 가스를 소모할 수도 있다고 하는데 그건 잘 모르겠다..) 또한 가장 중요한 점은 이미 실패한 트랜잭션인지 아닌지 조차 모른다는 것이다. 따라서 이러한 문제들을 방지하기 위해 트랜잭션 결과에 대한 정보를 저장하도록 코드를 수정해야 한다.