Bypass MAX_LOCK duration + External calls even when delegation is locked

[code423n4]

Lines of code

https://github.com/pooltogether/v4-twab-delegator/blob/master/contracts/Delegation.sol#L52 https://github.com/pooltogether/v4-twab-delegator/blob/master/contracts/Delegation.sol#L40

Vulnerability details

Impact

Delegation owner can change the MAX_LOCK duration even though current lock set on delegation has not yet expired

Also Delegation owner can execute calls even when delegation is locked

Proof of Concept

Bypass MAX_LOCK:

1. User A creates a new Delegation using createDelegation function at TWABDelegator.sol#L226. User A has set _lockDuration as 1 year.

2. The delegation gets created and delegation _owner is set as User A

3. User A wants to use updateDelegatee function but since _lockDuration is not over so the function fails

4. User A simply calls the setLockUntil function at Delegation.sol#L52 (address of this is returned in step 1). Since User A is owner of this delegation, he is allowed to call this function

5. Since MAX_LOCK check is not present in setLockUntil function, User A simply sets the lock timestamp as 0 and goes ahead with updateDelegatee function at TWABDelegator.sol#L258 which now works

Execute Calls with locked Delegation:

1. User A creates a new Delegation using createDelegation function at TWABDelegator.sol#L226. User A has set _lockDuration as 1 year.

2. The delegation gets created and delegation _owner is set as User A

3. User A wants to use update Delegatee or transfer function but since _lockDuration is not over so the function fails

4. User A simply calls the executeCalls function with required selector on Delegation.sol#L40 which works since User A is owner of this delegation

Recommended Mitigation Steps

Add a modifier check on both setLockUntil and executeCalls so that these are only callable from TWABDelegator.sol contract

[PierrickGT]

This attack scenario is not possible.

For Bypass MAX_LOCK:

User A can't directly call the setLockUntil function since we create the delegation via CREATE2. https://github.com/pooltogether/v4-twab-delegator/blob/master/contracts/TWABDelegator.sol#L237-L240 https://github.com/pooltogether/v4-twab-delegator/blob/master/contracts/LowLevelDelegator.sol#L28-L32

The owner is set as msg.sender, so the only address who can call the setLockUntil function is the TwabDelegator contract itself. https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/Delegation.sol#L31 https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/Delegation.sol#L52

In order to interact with the setLockUntil function, a user has to interact with the TwabDelegator contract. The only place where we call the setLockUntil function is in the updateDelegatee function: https://github.com/pooltogether/v4-twab-delegator/blob/master/contracts/TWABDelegator.sol#L274

In this function, we do check if the delegation is unlocked before allowing the user to update the lock duration: https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/TWABDelegator.sol#L269

For Execute Calls with locked Delegation:

As explained above, in order to interact with the executeCalls function, a user has to interact with the TwabDelegator contract, since we have a onlyOwner modifier on the function and the owner is the TwabDelegator contract. https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/Delegation.sol#L40

The executeCalls function is called in the internal function _executeCall and this function only interacts with the ticket contract. https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/TWABDelegator.sol#L555 https://github.com/pooltogether/v4-twab-delegator/blob/21bb53b2ea54a248bbd1d3170dbadd3a0c83d874/contracts/TWABDelegator.sol#L553

So no way to update the lock duration through this function.

For the above reasons, I've disputed the issue.

[0xleastwood]

I agree with the sponsor.

Considering the two POCs outlined by the warden:

• Users cannot arbitrarily call setLockUntil() as the onlyOwner role is held by the TWABDelegator.sol contract. This function is only executed in updateDelegatee() and requires that the delegation is in an unlocked state.
• Similarly, the same user cannot call executeCalls() bypass the locked delegation by transferring tokens out of the contract.

As per my considerations and the sponsor's comments above, I'll mark this as invalid.