QA report

[code423n4]

Lines of code

https://github.com/pooltogether/v4-twab-delegator/blob/master/contracts/TWABDelegator.sol#L188
https://github.com/pooltogether/v4-twab-delegator/blob/master/contracts/TWABDelegator.sol#L204

Vulnerability details

Impact

In TWABDelegator.sol the stake() and unstake() functions handle the minting and burning of tickets to and from the caller. The _to address argument on both is the address to which the stake will be attributed but both functions do not make sure that the _to argument is not the TWABDelegator.sol contract itself. These checks should be added to avoid leaving open areas in which the protocol may be manipulated.

Proof of Concept

https://github.com/pooltogether/v4-twab-delegator/blob/master/contracts/TWABDelegator.sol#L188

https://github.com/pooltogether/v4-twab-delegator/blob/master/contracts/TWABDelegator.sol#L204

Tools Used

Manual code review

Recommended Mitigation Steps

Add to stake() and unstake() functions:
require(_to != address(this), "Cannot be TwabDelegator");

[PierrickGT]

PR: pooltogether/v4-twab-delegator#27

[0xleastwood]

I'm not convinced this should be rated as medium severity. The warden has not justified why this might cause the protocol issues. I think this could be better rated as low or non-critical. @PierrickGT

[PierrickGT]

I'm not convinced this should be rated as medium severity. The warden has not justified why this might cause the protocol issues. I think this could be better rated as low or non-critical. @PierrickGT

I agree. Per Brendan comment, we are not going to implement this suggestion since only users interacting directly with the contract could make this mistake. So I've added the sponsor acknowledged badge.
As for the severity, it would result in a loss of fund if the user was to pass the contract address. But since this scenario is pretty unlikely, we could rate is as low or dispute the issue entirely.

[0xleastwood]

As per the above comment, I will mark this as low and have it be part of the QA report.

[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 "No check that _to address is not the contract itself."