code423n4 commented 1 year ago

Lines of code

Vulnerability details

Impact

Anyone interacting with Pooltogether may wrongly assume that functions handle rounding according to the ERC4626 standard. Therefore, it may cause some integration problems in the future which can lead to many problems for both parties.

Proof of Concept

I) Math rounding in Vault.sol is not ERC4626-complicant: _convertToAssets() should round up and _convertToShares() should round down

Per EIP 4626's Security Considerations :
Finally, ERC-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users:

_convertToAssets() fits the description when is required to round up:

If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

_convertToShares() fits the description when is required to round down :

If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down.

File: Vault.sol
313:    uint256 _sharesToAssets = _convertToAssets(_totalShares(), Math.Rounding.Down); // @audit should be rounding up
----
531:    uint256 _assets = _convertToAssets(_shares, Math.Rounding.Down); // @audit should be rounding up
----
1170:    uint256 _totalSupplyToAssets = _convertToAssets(
1171:      _totalSupplyAmount,
1172:      _lastRecordedExchangeRate,
1173:      Math.Rounding.Down // @audit should be rounding up
1174:    );

Vault.sol#L313 , Vault.sol#L531 , Vault.sol#L1170-L1174

File: Vault.sol
517:    uint256 _shares = _convertToShares(_assets, Math.Rounding.Up); // @audit should be rounding down

Vault.sol#L517

II) maxWithdraw and maxRedeem does not completely adhere to EIP-4626

File: Vault.sol
529:    if (_shares > maxRedeem(_owner)) revert RedeemMoreThanMax(_owner, _shares, maxRedeem(_owner));
----
514:    if (_assets > maxWithdraw(_owner))
515:      revert WithdrawMoreThanMax(_owner, _assets, maxWithdraw(_owner));
----
801:    return _yieldVault.maxWithdraw(address(this)) + super.totalAssets();
----
1176:    uint256 _withdrawableAssets = _yieldVault.maxWithdraw(address(this));

The current implementations of maxWithdraw and maxRedeem do not follow this specification: LINK TO CODE

123    function maxWithdraw(address owner) public view virtual override returns (uint256) {
124:        return _convertToAssets(balanceOf(owner), Math.Rounding.Down);
125:    }
-----
128:    function maxRedeem(address owner) public view virtual override returns (uint256) {
129:        return balanceOf(owner);
130:    }

EIP-4626 specifies concerning maxWithdraw :

MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0.

and maxRedeem :

MUST factor in both global and user-specific limits, like if redemption is entirely disabled (even temporarily) it MUST return 0.

The implementation does not conform to this specification. Except for first Tuesday of each month, withdraws and redeems are disabled. In such a case, these functions MUST return 0.

Tools Used

Manual Audit and EIP-4626 documentation

Recommended Mitigation Steps

Stick to the EIP-4626 documentation and make sure to round up in the _convertToAssets() and round down in _convertToShares().
Check when the withdraw and redeems are disabled and return 0 if so.

Assessed type

ERC4626

c4-sponsor commented 1 year ago

asselstine marked the issue as sponsor confirmed

Picodes commented 1 year ago

" Except for first Tuesday of each month, withdraws and redeems are disabled. In such a case, these functions MUST return 0." -> I don't get this part of the report.