PoolTogether TWAB Delegator contest - WatchPug's results

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.

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S] Move lockUntil from Delegation to TWABDelegator can save gas from external call

The lockUntil of a Delegation is currently stored on the Delegation contract and usually used by the TWABDelegator contract through an external call (_delegation.lockUntil()).

https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/TWABDelegator.sol#L624-L626

  function _requireDelegationUnlocked(Delegation _delegation) internal view {
    require(block.timestamp >= _delegation.lockUntil(), "TWABDelegator/delegation-locked");
  }

In comparison to storing the lockUntil on TWABDelegator contract, this creates an overhead of one external call.

[M] Use custom error may save gas

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

[M] ++i is more gas efficient than i++

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

[N] "> 0" is less efficient than "!= 0" for unsigned integers

https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/TWABDelegator.sol#L601-L601