PoolTogether - Aymen0909's results

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L550-L587

Vulnerability details

Impact

In the liquidate function from the Vault contract, the input argument _amountOut is used as if it was representing a value of asset amount and share amount at the same time which is impossible a there a conversion rate between them, this error will make liquidate function behave in an expected manner, not the one that was intended.

Proof of Concept

The issue is occurring in the liquidate function below :

function liquidate(
    address _account,
    address _tokenIn,
    uint256 _amountIn,
    address _tokenOut,
    uint256 _amountOut
) public virtual override returns (bool) {
    _requireVaultCollateralized();
    if (msg.sender != address(_liquidationPair))
      revert LiquidationCallerNotLP(msg.sender, address(_liquidationPair));
    if (_tokenIn != address(_prizePool.prizeToken()))
      revert LiquidationTokenInNotPrizeToken(_tokenIn, address(_prizePool.prizeToken()));
    if (_tokenOut != address(this))
      revert LiquidationTokenOutNotVaultShare(_tokenOut, address(this));
    if (_amountOut == 0) revert LiquidationAmountOutZero();
    
    uint256 _liquidableYield = _liquidatableBalanceOf(_tokenOut);
    
    // @audit _amountOut compared with _liquidableYield which represents an asset amount
    // @audit _amountOut is considered as an asset amount
    if (_amountOut > _liquidableYield)
      revert LiquidationAmountOutGTYield(_amountOut, _liquidableYield);
    
    _prizePool.contributePrizeTokens(address(this), _amountIn);
    
    if (_yieldFeePercentage != 0) {
        // @audit _amountOut used to increase fee shares so considered as representing a share amount
        _increaseYieldFeeBalance(
            (_amountOut * FEE_PRECISION) / (FEE_PRECISION - _yieldFeePercentage) - _amountOut
        );
    }
    
    uint256 _vaultAssets = IERC20(asset()).balanceOf(address(this));
    
    // @audit _amountOut compared with _vaultAssets which represents an asset amount
    // @audit _amountOut is considered as an asset amount
    if (_vaultAssets != 0 && _amountOut >= _vaultAssets) {
      _yieldVault.deposit(_vaultAssets, address(this));
    }
    
    // @audit _amountOut representing a share amount minted to the _account
    _mint(_account, _amountOut);
    
    return true;
}

As you can see from the code above, the value of the argument _amountOut is used multiple times in the function logic and each time it is representing either an asset amount or a share amount which is impossible as there a conversion formula used to transform asset amount into share amount (and inversely) with the function _convertToShares (or _convertToAssets).

From the function comments i couldn't figure out what the value of _amountOut actually represents, but because there is also another argument given to the liquidate function which is _tokenOut == address(this), I'm supposing that _amountOut is representing a share amount which will mean that all the instances highlighted in the code above when _amountOut is considered as an asset amount are wrong.

• Instance 1 :

// @audit _amountOut compared with _liquidableYield which represents an asset amount
if (_amountOut > _liquidableYield)
  revert LiquidationAmountOutGTYield(_amountOut, _liquidableYield);

• Instance 2 :

// @audit _amountOut compared with _vaultAssets which represents an asset amount
if (_vaultAssets != 0 && _amountOut >= _vaultAssets) {
  _yieldVault.deposit(_vaultAssets, address(this));
}

And before comparing _amountOut to the asset amount values : _vaultAssets and _liquidableYield, its value should be converted to an asset amount with the function _convertToAssets.

This issue will cause problems for the protocol working as the liquidate function logic will not behave as expected (because it's comparing values that represents different things).

** Note : if _amountOut is actually representing an asset amount (not a share amount as i supposed), the issue is still valid because _amountOut is also used as being a share amount inside the liquidate function, in that case it should first be converted to a share amount with _convertToShares in order to get the correct behavior of the liquidate function.

Tools Used

Manual review

Recommended Mitigation Steps

To solve this issue i recommend to first convert the value of _amountOut in the liquidate function to an asset amount and store it in a local variable _amountOutToAsset, and in the function logic use the correct variable (either _amountOut or _amountOutToAsset) when interacting with a share amount or an asset amount.

Assessed type

Error

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L394-L402

Vulnerability details

Impact

The mintYieldFee function inside the Vault contract can be called by anyone to transfer the accrued yield fees shares to a given _recipient address (set by the user), but the function is normally supposed to transfer those fees to the _yieldFeeRecipient set by the contract owner.

Proof of Concept

The issue occurs in the mintYieldFee function below :

File: Vault.sol Line 394-402

function mintYieldFee(uint256 _shares, address _recipient) external {
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);
    
    _yieldFeeTotalSupply -= _shares; 
    // @audit The fees shares should be minted to `_yieldFeeRecipient`
    _mint(_recipient, _shares);
    
    emit MintYieldFee(msg.sender, _recipient, _shares);
}

As you can see from the code above, the function has no access control (modifier or check on the caller), it allows the caller to give any _recipient address which the fee shares get minted to (when the vault is collateralized, of course).

But from the comments we can find that all the fees captured should go to the _yieldFeeRecipient and don't to a random user :

/// @notice Address of the yield fee recipient that receives the fee amount when yield is captured.
address private _yieldFeeRecipient;

The impact of this issue is that all the accrued fee shares are distributed to random users who make a call to the mintYieldFee function instead of the _yieldFeeRecipient. And in order to make sure that the fees are minted to the _yieldFeeRecipient, the protocol devs would have to monitor at each moment when the vault is collateralized and try to be the first to call the mintYieldFee function which not suitable and maybe impossible.

Tools Used

Manual review

Recommended Mitigation Steps

To solve this issue i recommend to remove the _recipient address argument from the mintYieldFee function and mint directly to the _yieldFeeRecipient address.

The code should become :

function mintYieldFee(uint256 _shares) external {
    _requireVaultCollateralized();
    if (_shares > _yieldFeeTotalSupply) revert YieldFeeGTAvailable(_shares, _yieldFeeTotalSupply);
    
    _yieldFeeTotalSupply -= _shares; 
    // @audit any user that call the function will mint to `_yieldFeeRecipient`
    _mint(_yieldFeeRecipient, _shares);
    
    emit MintYieldFee(msg.sender, _yieldFeeRecipient, _shares);
}

Assessed type

Token-Transfer

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L480-L482 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L494-L504 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L982-L994

Vulnerability details

Impact

An attacker can change the delegatee of a user who deposited into the vault to the SPONSORSHIP_ADDRESS address by calling one of the functions sponsor or sponsorWithPermit and giving the address of the user as _receiver.

The impact of this issue is that the attacker can stop users from winning prizes because the users who have delegated to the SPONSORSHIP_ADDRESS address get their chances of winning prizes revoked.

Proof of Concept

As the functions sponsor and sponsorWithPermit do the same thing except the later include a permit functionality which doesn't have an impact on this issue, i will explain how this issue can be exploited with the sponsor function and the same apply to the sponsorWithPermit function.

We start with the sponsor function :

function sponsor(uint256 _assets, address _receiver) external returns (uint256) {
    return _sponsor(_assets, _receiver);
}

As you can see the function can be called by anyone, and it takes the _assets amount of asset to deposit into the vault and a _receiver address that the user wants to deposit to, the function calls the internal function _sponsor below :

function _sponsor(uint256 _assets, address _receiver) internal returns (uint256) {
    uint256 _shares = deposit(_assets, _receiver);

    if (
      _twabController.delegateOf(address(this), _receiver) != _twabController.SPONSORSHIP_ADDRESS()
    ) {
      _twabController.sponsor(_receiver);
    }
    
    emit Sponsor(msg.sender, _receiver, _assets, _shares);
    
    return _shares;
}

Inside this function we can see that it call the deposit function for performing the deposit of the asset amount, and then the function checks the current delegatee of the _receiver address, if this one is different from the SPONSORSHIP_ADDRESS address the function will call the _twabController.sponsor function to changes the _receiver delegatee address to the SPONSORSHIP_ADDRESS address :

// TwabController.sponsor
function sponsor(address _from) external {
    _delegate(msg.sender, _from, SPONSORSHIP_ADDRESS);
}

In the comments of the TwabController contract we can see the following :

/// @notice Allows users to revoke their chances to win by delegating to the sponsorship address.
address public constant SPONSORSHIP_ADDRESS = address(1);

Basically any users who is delegating to the SPONSORSHIP_ADDRESS address is not included in the prize distribution and has no chances to win.

So to resume any attacker can change all other users delegations to the SPONSORSHIP_ADDRESS address and deprive them from the chance of winning prizes, and this attack will not cost the attacker any funds as he can at each time deposit a very small amount to every user when calling the sponsor function.

The scenario of the attack can be summarized as follows :

• Alice has deposited funds into a given Vault, and she has not delegated them so in the TwabController contract she is basically delegating to her self meaning : delegates[_vault][Alice] = Alice.

• The attacker Bob sees that Alice has deposited funds into the vault, so he calls the sponsor function with the following parameters : _assets = 10 (choose 10 just for the example but could be any other very small amount) and _receiver = Alice.

• The sponsor function after calling deposit to deposit _assets = 10 to Alice, proceeds to check the current delegatee of Alice and it finds that it is different from the SPONSORSHIP_ADDRESS address : delegates[_vault][Alice] = Alice != SPONSORSHIP_ADDRESS

• So it calls the _twabController.sponsor function to change Alice's delegated address to SPONSORSHIP_ADDRESS.

• The attack now is over and Bob managed to change Alice delegatee to SPONSORSHIP_ADDRESS meaning that now she is not included in the prize distribution anymore (has 0 chances to win).

Alice could find out about this after a while and change the delegation but until that time she will be thinking that she's participating in the lottery when in fact she's not. And Bob can repeat the same attack on other users who also deposited into the Vault.

Tools Used

Manual review

Recommended Mitigation Steps

There are many ways to solve this issue but the simplest one is to add a check in the _sponsor function which changes the delegation only if : _receiver == msg.sender.

The function _sponsor should be updated as follows :

function _sponsor(uint256 _assets, address _receiver) internal returns (uint256) {
    uint256 _shares = deposit(_assets, _receiver);
    // @audit only change delegation when caller is same as _receiver
    if (
      msg.sender == _receiver &&
      _twabController.delegateOf(address(this), _receiver) != _twabController.SPONSORSHIP_ADDRESS()
    ) {
      _twabController.sponsor(_receiver);
    }
    
    emit Sponsor(msg.sender, _receiver, _assets, _shares);
    
    return _shares;
}

Assessed type

Other

Lines of code

https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L278-L321 https://github.com/GenerationSoftware/pt-v5-twab-controller/blob/0145eeac23301ee5338c659422dd6d69234f5d50/src/libraries/TwabLib.sol#L254-L276

Vulnerability details

Impact

The Natspec of both getBalanceAt and getTwabBetween functions indicates that they should implement the functions isTimeSafe and isTimeRangeSafe (respectively) to ensure that the queried timestamps are safe, but both functions don't implement them which can lead both functions to return wrong values which in the end will impact the protocol accounting as those function are used to fetch the users balances fro a given period.

Proof of Concept

I will start by explaining what the functions isTimeSafe and isTimeRangeSafe actually do. The isTimeRangeSafe is basically a double call to the function isTimeSafe for the start and end timestamps provided to it, so to understand what both functions are doing we should look at isTimeSafe :

function _isTimeSafe(
    uint32 PERIOD_LENGTH,
    uint32 PERIOD_OFFSET,
    ObservationLib.Observation[MAX_CARDINALITY] storage _observations,
    AccountDetails memory _accountDetails,
    uint32 _time
) private view returns (bool) {
    // @audit 1- check when there is no observations
    // If there are no observations, it's an unsafe range
    if (_accountDetails.cardinality == 0) {
      return false;
    }
    // If there is one observation, compare it's timestamp
    uint32 period = _getTimestampPeriod(PERIOD_LENGTH, PERIOD_OFFSET, _time);
    
    // @audit 1- check when there is a single observation
    if (_accountDetails.cardinality == 1) {
      return
        period != _getTimestampPeriod(PERIOD_LENGTH, PERIOD_OFFSET, _observations[0].timestamp)
          ? true
          : _time >= _observations[0].timestamp;
    }
    ObservationLib.Observation memory preOrAtObservation;
    ObservationLib.Observation memory nextOrNewestObservation;
    
    // @audit 2- check on the newest observation
    (, nextOrNewestObservation) = getNewestObservation(_observations, _accountDetails);
    
    if (_time >= nextOrNewestObservation.timestamp) {
      return true;
    }
    
    // @audit 3- check on the period of the oberservations after & before
    (preOrAtObservation, nextOrNewestObservation) = _getSurroundingOrAtObservations(
      PERIOD_OFFSET,
      _observations,
      _accountDetails,
      _time
    );
    
    uint32 preOrAtPeriod = _getTimestampPeriod(
      PERIOD_LENGTH,
      PERIOD_OFFSET,
      preOrAtObservation.timestamp
    );
    uint32 postPeriod = _getTimestampPeriod(
      PERIOD_LENGTH,
      PERIOD_OFFSET,
      nextOrNewestObservation.timestamp
    );
    
    // The observation after it falls in a new period
    return period >= preOrAtPeriod && period < postPeriod;
}

The functions working can be resumed into performing 3 checks :

1- A check in the case there is no observations or just a single one.

2- A check on the newest observation to see if the target time is after it.

3- A check to see if the target time is in a period between the old period and the next period.

We can see in the Natspec on the getBalanceAt function that it is supposed to use isTimeSafe to ensure timestamps are safe ;

 /**
    * @notice Looks up a users balance at a specific time in the past.
    * @dev If the time is not an exact match of an observation, the balance is extrapolated using the previous observation.
    * @dev Ensure timestamps are safe using isTimeSafe or by ensuring you're querying a multiple of the observation period intervals.
    * @param _observations The circular buffer of observations
    * @param _accountDetails The account details to query with
    * @param _targetTime The time to look up the balance at
    * @return balance The balance at the target time
*/

function getBalanceAt(
    uint32 PERIOD_OFFSET,
    ObservationLib.Observation[MAX_CARDINALITY] storage _observations,
    AccountDetails memory _accountDetails,
    uint32 _targetTime
) internal view returns (uint256) {
    // @audit not using isTimeSafe
    ObservationLib.Observation memory prevOrAtObservation = _getPreviousOrAtObservation(
      PERIOD_OFFSET,
      _observations,
      _accountDetails,
      _targetTime
    );
    return prevOrAtObservation.balance;
 }

And also in the Natspec of the getTwabBetween function, it is said that the function isTimeRangeSafe should be used to ensure the timestamps are safe :

/**
    * @notice Looks up a users TWAB for a time range.
    * @dev If the timestamps in the range are not exact matches of observations, the balance is extrapolated using the previous observation.
    * @dev Ensure timestamps are safe using isTimeRangeSafe.
    * @param _observations The circular buffer of observations
    * @param _accountDetails The account details to query with
    * @param _startTime The start of the time range
    * @param _endTime The end of the time range
    * @return twab The TWAB for the time range
*/

function getTwabBetween(
    uint32 PERIOD_OFFSET,
    ObservationLib.Observation[MAX_CARDINALITY] storage _observations,
    AccountDetails memory _accountDetails,
    uint32 _startTime,
    uint32 _endTime
) internal view returns (uint256) {
    // @audit did not use isTimeRangeSafe

    ObservationLib.Observation memory startObservation = _getPreviousOrAtObservation(
      PERIOD_OFFSET,
      _observations,
      _accountDetails,
      _startTime
    );
    
    ObservationLib.Observation memory endObservation = _getPreviousOrAtObservation(
      PERIOD_OFFSET,
      _observations,
      _accountDetails,
      _endTime
    );
    
    if (startObservation.timestamp != _startTime) {
      startObservation = _calculateTemporaryObservation(startObservation, _startTime);
    }
    
    if (endObservation.timestamp != _endTime) {
      endObservation = _calculateTemporaryObservation(endObservation, _endTime);
    }
    
    // Difference in amount / time
    return
      (endObservation.cumulativeBalance - startObservation.cumulativeBalance) /
      (_endTime - _startTime);
}

But as we can see both code snippets above both isTimeSafe and isTimeRangeSafe were not used in either of the functions.

We can also notice that both function make calls to the _getPreviousOrAtObservation function, and by going through its logic we can easily see that it contains the first two checks implemented by the function isTimeSafe but it doesn't contain the last check :

function _getPreviousOrAtObservation(
    uint32 PERIOD_OFFSET,
    ObservationLib.Observation[MAX_CARDINALITY] storage _observations,
    AccountDetails memory _accountDetails,
    uint32 _targetTime
) private view returns (ObservationLib.Observation memory prevOrAtObservation) {
    uint32 currentTime = uint32(block.timestamp);
    
    uint16 oldestTwabIndex;
    uint16 newestTwabIndex;
    
    // @audit 1- check when there is no observations
    // If there are no observations, return a zeroed observation
    if (_accountDetails.cardinality == 0) {
      return
        ObservationLib.Observation({ cumulativeBalance: 0, balance: 0, timestamp: PERIOD_OFFSET });
    }
    
    // @audit 2- check on the newest observation
    // Find the newest observation and check if the target time is AFTER it
    (newestTwabIndex, prevOrAtObservation) = getNewestObservation(_observations, _accountDetails);
    if (_targetTime >= prevOrAtObservation.timestamp) {
      return prevOrAtObservation;
    }
    
    // @audit 1- check when there is a single observation
    // If there is only 1 actual observation, either return that observation or a zeroed observation
    if (_accountDetails.cardinality == 1) {
      if (_targetTime >= prevOrAtObservation.timestamp) {
        return prevOrAtObservation;
      } else {
        return
          ObservationLib.Observation({
            cumulativeBalance: 0,
            balance: 0,
            timestamp: PERIOD_OFFSET
          });
      }
    }
    
    // @audit a check on the oldest observation also implemented in `_isTimeSafe`
    // Find the oldest Observation and check if the target time is BEFORE it
    (oldestTwabIndex, prevOrAtObservation) = getOldestObservation(_observations, _accountDetails);
    if (_targetTime < prevOrAtObservation.timestamp) {
      return
        ObservationLib.Observation({ cumulativeBalance: 0, balance: 0, timestamp: PERIOD_OFFSET });
    }
    
    ObservationLib.Observation memory afterOrAtObservation;
    // Otherwise, we perform a binarySearch to find the observation before or at the timestamp
    (prevOrAtObservation, afterOrAtObservation) = ObservationLib.binarySearch(
      _observations,
      newestTwabIndex,
      oldestTwabIndex,
      _targetTime,
      _accountDetails.cardinality,
      currentTime
    );
    
    // If the afterOrAt is at, we can skip a temporary Observation computation by returning it here
    if (afterOrAtObservation.timestamp == _targetTime) {
      return afterOrAtObservation;
    }
    
    return prevOrAtObservation;
}

Because the function _getPreviousOrAtObservation does not contain the last check on the periods, both functions getBalanceAt and getTwabBetween can return a wrong result, and because they are used by the protocol to fetch the balance of users (or vaults), this issue can cause problems to the good working of the system.

Tools Used

Manual review

Recommended Mitigation Steps

To resolve this issue i recommend to add a check on the previous and next periods found in _getPreviousOrAtObservation, this will ensure that the function will behave as does the function isTimeSafe and will ensure that the queried timestamps are safe in both getBalanceAt and getTwabBetween.

Assessed type

Invalid Validation