PoolTogether V5: Part Deux - Arz's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L131

Vulnerability details

For a draw auction to complete, a bot must relay a completed RNG result to the Prize Pool. This is done by calling rngComplete() and the data must originate from the relayer. However the rngComplete() function is missing a check that the msg.sender is the relayer so this allows anyone to call this function and pass in any data.

Impact

The attacker has the ability to input their own data so he can input his own random number that he will benefit from and ensure that he will win. The attacker can also take advantage of the rewards that he will get and he can make them as big as possible to drain the reserves.

Proof of Concept

https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L77-L79

77:   /// @notice The relayer that RNG results must originate from.
78:   /// @dev Note that this may be a Remote Owner if relayed over an ERC-5164 bridge.
79:   address public immutable rngAuctionRelayer;

As you can see here the rngAuctionRelayer is defined and the comments specify that only the relayer should have the privilege to call rngComplete().

However rngComplete() is missing this check:

https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L131-L138

131: function rngComplete(
132:   uint256 _randomNumber,
133:   uint256 _rngCompletedAt,
134:   address _rewardRecipient,
135:   uint32 _sequenceId,
136:   AuctionResult calldata _rngAuctionResult
137: ) external returns (bytes32) {
138:   if (_sequenceHasCompleted(_sequenceId)) revert SequenceAlreadyCompleted();

The attacker can input his own data like the randomNumber,rewardFraction and etc. The only thing that he has to make sure is that he inputs the correct sequence id.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a require statement that ensures that RngRelayAuction::rngComplete() is only callable by the auction relayer

Assessed type

Access Control