PoolTogether TWAB Delegator contest - Omik's results

LOW : 1. Title : permitAndMulticall() can be frontrun, that will lead to the user must reasign the TX

Impact : In the permitAndMulticall() it takes from as a user input, since all tx in the blockchain is public, a malicious user might frontrun the permitAndMulticall() and providing 0 data, which will increment the nonce of the from user in the erc20permit, by the time the original from user tx get executed it will be revert since the nonce already inflate, and the original user must re-sign.

POC : https://github.com/pooltogether/v4-twab-delegator/blob/2b6d42506187dd7096043e2dfec65fa06ab18577/contracts/TWABDelegator.sol#L444

Mitigation : remove the from parameter, and change the from to msg.sender.

Title : Code didnt match with the documentation for representative flow

Impact : In the https://dev.pooltogether.com/protocol/contracts/v4-twab-delegator/ it explain that the the delegator must stake first before the representative can createDelegation, but in the https://github.com/pooltogether/v4-twab-delegator/blob/2b6d42506187dd7096043e2dfec65fa06ab18577/contracts/TWABDelegator.sol#L400 the delegator can set a representative and the code didnt check if the delegator already have stake a token or not, after setting a representative the representative than allow to call createDelegation with the delegator stake a token first.

POC : https://github.com/pooltogether/v4-twab-delegator/blob/2b6d42506187dd7096043e2dfec65fa06ab18577/contracts/TWABDelegator.sol#L400

GAS : 1. Title : using uint96 in a struct can save gas

Impact : By changing the call struct inside delegation contract from using uint256 to uint96 can save some gas, because the current struct is using 3 slot storage, by changing it to uint96, the address and uint96 will be placed in the same slot, therefore it only take 2 slot spaces.

POC : https://github.com/pooltogether/v4-twab-delegator/blob/2b6d42506187dd7096043e2dfec65fa06ab18577/contracts/Delegation.sol#L15

Title : unnecesary check on _requireDelegatorOrRepresentative()

Impact : There is no need to check if the representatives[_delegator][msg.sender] == true, because if the delegator already set the msg.sender as the representative the value would be true, by removing == true will save some gas

POC : https://github.com/pooltogether/v4-twab-delegator/blob/2b6d42506187dd7096043e2dfec65fa06ab18577/contracts/TWABDelegator.sol#L583

Mitigation :

  function _requireDelegatorOrRepresentative(address _delegator) internal view {
    require(
      _delegator == msg.sender || representatives[_delegator][msg.sender],
      "TWABDelegator/not-delegator-or-rep"
    );
  }