QA report

[code423n4]

Lines of code

https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/Delegation.sol#L39-L46

Vulnerability details

Delegation is a contract deployed dedicated to holding the ticket tokens for the delegator and they can then be delegate to a delegatee.

On the Delegation contract, there is a method named executeCalls() designed for "Executes calls on behalf of this contract" which allows arbitrary code execution for the owner.

However, we found that the owner of Delegation will always be TWABDelegator, and the TWABDelegator will only use Delegation.sol#executeCalls() to call one particular address: the ticket address, and for only two methods: transfer() and delegate().

Furthermore, even though in Delegation.sol#executeCalls(), calls[i].value is used, the function is not being marked as payable, that makes it hard for calls that requires eth payments.

https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/Delegation.sol#L39-L46

  function executeCalls(Call[] calldata calls) external onlyOwner returns (bytes[] memory) {
    bytes[] memory response = new bytes[](calls.length);
    for (uint256 i = 0; i < calls.length; i++) {
      response[i] = _executeCall(calls[i].to, calls[i].value, calls[i].data);
    }
    return response;
  }

While the ticket is being delegated through TWABDelegator, they won't be able to retrieve the tickets back until the lockUntil, without the ability to make arbitrary code execution, the delegator may miss some of the potential benefits as a holder of the ticket tokens, for example, an airdrop to all holders of the ticket tokens, or an NFT made mintable only for certain ticket holders.

Recommendation

Consider adding a new method on TWABDelegator:

function executeCalls(
  address _delegator,
  uint256 _slot,
  Delegation.Call[] memory calls
) external payable returns (bytes[] memory) {
  _requireDelegatorOrRepresentative(_delegator);
  Delegation _delegation = Delegation(_computeAddress(_delegator, _slot));

  if (block.timestamp < _delegation.lockUntil()) {
    for (uint256 i = 0; i < calls.length; i++) {
      if (calls[i].to == address(ticket)) {
        revert("TWABDelegator/delegation-locked");
      }
    }
  }

  return _delegation.executeCalls{value: msg.value}(_calls);
}

And also, consider making Delegation.sol#executeCalls() a payable method.

[PierrickGT]

The issue outlined by the warden is relevant but users won't need to execute arbitrary calls cause potential rewards given out to ticket holders will be handled by our TWABRewards contract. This contract retrieves users TWAB (Time-Weighted Average Balance) for a given period of time and calculate the amount of rewards they are eligible to. Users can then claimRewards on behalf of others. So delegatees will be able to claim their rewards and delegators could claim on their behalf.

https://github.com/pooltogether/v4-periphery/blob/348d2bf7cfcf5750bad4aae63b8ade5a2a45f188/contracts/TwabRewards.sol#L410 https://github.com/pooltogether/v4-periphery/blob/348d2bf7cfcf5750bad4aae63b8ade5a2a45f188/contracts/interfaces/ITwabRewards.sol#L94

For more informations about how the TWAB works, here is some documentation:

• https://dev.pooltogether.com/protocol/architecture/time-weighted-average-balance
• https://medium.com/pooltogether/better-reward-distribution-65d900f3cef0

Also, by restricting calls to the transfer and delegate` methods on the ticket, we limit the attack surface and any attack vector we may not have thought about.

For the reasons above, I've acknowledged the issue but we won't implement the proposed solution

[0xleastwood]

I don't really see a case where _delegateCall() or _transferCall() will need to have some ETH attached with it. They are solely dealing with the Ticket ERC20 token and updating delegation data. Considering the fact that rewards are handled by a separate contract, I think its fair to downgrade this to 1 (Low Risk).

[CloudEllie]

Since this issue was downgraded to a QA level, and the warden did not submit a separate QA report, we've renamed this one to "QA report" for consistency.

The original title, for the record, was "[WP-M1] delegator and/or representative should be allowed for arbitrary code execution besides restricted operations during unlocked period."