code423n4 commented 1 year ago

Lines of code

https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngRelayAuction.sol#L170 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngAuction.sol#L170 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngAuctionRelayerDirect.sol#L36 https://github.com/GenerationSoftware/pt-v5-draw-auction/blob/f1c6d14a1772d6609de1870f8713fb79977d51c1/src/RngAuctionRelayerRemoteOwner.sol#L59

Vulnerability details

Impact

The rngComplete() function receives the RNG request results, it closes the draw using the randomNumber generated by the RNG request auction and it transfer the rewards.

The problem is that a malicious actor can introduce a address zero in the recipient reward causing the revert of the rngComplete() function, this will cause a destabilization because:

Draws will not be closed causing problems for the winners to claim prizes
The protocol will be unable to close draws and start the next one.

Proof of Concept

In the RNG request auction there is not any restriction about to use address(0) in the _rewardRecipient parameter.

  function startRngRequest(address _rewardRecipient) external {
    if (!_canStartNextSequence()) revert CannotStartNextSequence();

    RNGInterface rng = _nextRng;

    uint64 _auctionElapsedTimeSeconds = _auctionElapsedTime();
    if (_auctionElapsedTimeSeconds > auctionDuration) revert AuctionExpired();

    (address _feeToken, uint256 _requestFee) = rng.getRequestFee();
    if (_feeToken != address(0) && _requestFee > 0) {
      if (IERC20(_feeToken).balanceOf(address(this)) < _requestFee) {
        // Transfer tokens from caller to this contract before continuing
        IERC20(_feeToken).transferFrom(msg.sender, address(this), _requestFee);
      }
      // Increase allowance for the RNG service to take the request fee
      IERC20(_feeToken).safeIncreaseAllowance(address(rng), _requestFee);
    }

    (uint32 rngRequestId,) = rng.requestRandomNumber();
    uint32 sequenceId = _openSequenceId();
    UD2x18 rewardFraction = _currentFractionalReward();

    _lastAuction = RngAuctionResult({
      recipient: _rewardRecipient,
      rewardFraction: rewardFraction,
      sequenceId: sequenceId,
      rng: rng,
      rngRequestId: rngRequestId
    });

    emit RngAuctionCompleted(
      _rewardRecipient,
      sequenceId,
      rng,
      rngRequestId,
      _auctionElapsedTimeSeconds,
      rewardFraction
    );
  }

Additionally, the relayer process can be to other chains using the 5164 message dispatcher. So a malicious actor can introduce an address(0) in the rewardRecipient parameter causing the rngComplete() revert in the specified chain.

    function relay(
        IRngAuctionRelayListener _remoteRngAuctionRelayListener,
        address rewardRecipient
    ) external returns (bytes32) {
        bytes memory listenerCalldata = encodeCalldata(rewardRecipient);
        bytes32 messageId = messageDispatcher.dispatchMessage(
            toChainId,
            address(account),
            RemoteOwnerCallEncoder.encodeCalldata(address(_remoteRngAuctionRelayListener), 0, listenerCalldata)
        );
        emit RelayedToDispatcher(rewardRecipient, messageId);
        return messageId;
    }

In the rngComplete() function, the reward process will revert the function when the reward recipient is zero. The transfer to address(0) reverts the transaction.

File: RngRelayAuction.sol
167:     for (uint8 i = 0; i < _rewards.length; i++) {
168:       uint104 _reward = uint104(_rewards[i]);
169:       if (_reward > 0) {
170:         prizePool.withdrawReserve(auctionResults[i].recipient, _reward);
171:         emit AuctionRewardDistributed(_sequenceId, auctionResults[i].recipient, i, _reward);
172:       }
173:     }

Tools used

Manual review

Recommended Mitigation Steps

Add an address zero validation for the rewardReicipient paramter in the startRngRequest() and the RngAuctionRelayerRemoteOwner.relay() functions. Additionally the remapTo() function must check the address zero in the _destination paramter.

Assessed type

Invalid Validation

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #92

raymondfam commented 1 year ago

The severity should be medium.

c4-judge commented 1 year ago

HickupHH3 changed the severity to 2 (Med Risk)