code-423n4 / 2023-07-pooltogether-findings

12 stars 7 forks source link

Anyone can mint to themselves type(uint96).max if _isVaultCollateralized() returns true #371

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

There is no check that ensures the caller to mint is a trusted one. Moreover, there is a flaw which lets anyone to mint type(uint96).max number of shares

Proof of Concept

First, the mint function does not implement any check for the caller to be someone with a role MINTER, OWNER or whatever (like in many other projects), thus it lets anyone to call it.

function mint(uint256 _shares, address _receiver) public virtual override returns (uint256) {
    uint256 _assets = _beforeMint(_shares, _receiver);

    _deposit(msg.sender, _receiver, _assets, _shares);

    return _assets;
  }

Second, the number of _assets is given by _beforeMint, which checks that _shares > maxMint(_receiver) for the _receiver

function _beforeMint(uint256 _shares, address _receiver) internal view returns (uint256) {
    if (_shares > maxMint(_receiver)) revert MintMoreThanMax(_receiver, _shares, maxMint(_receiver));
    return _convertToAssets(_shares, Math.Rounding.Up);
  }

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

However, it does NOT take into account the _receiver and will return type(uint96).max if the vault is collaterized (ALWAYS, even when _currentExchangeRate() == _assetUnit, making the exchange rate drop after the transaction due to the huge increment in one of the sides). Thus a malicious transaction could call the mint and give the caller "unlimited" tokens

Tools Used

Manual analysis

Recommended Mitigation Steps

First add the propper checks to the mint function to ensure the caller is a trusted one (requires and all of that). Second, do NOT let the maxMint function to proceed returning type(uint96).max without validating the _receiver parameter (either giving him an amount proportional to something, like its deposit, IDK, that's up to the devs)

Assessed type

Access Control

c4-judge commented 1 year ago

Picodes marked the issue as unsatisfactory: Invalid

Picodes commented 1 year ago

Anyone is supposed to be able to mint here