search

sherlock-audit / 2024-05-pooltogether-judging

13 stars 8 forks source link

AuditorPraise - `prizeVault.previewDeposit()` and `prizeVault.previewMint()` don't comply to ERC4626 standard #40

Closed sherlock-admin4 closed 4 months ago

sherlock-admin4 commented 4 months ago

AuditorPraise

medium

prizeVault.previewDeposit() and prizeVault.previewMint() don't comply to ERC4626 standard

Summary

prizeVault.previewDeposit() and prizeVault.previewMint() don't comply to ERC4626 standard

Vulnerability Detail

Is the codebase expected to comply with any EIPs? Can there be/are there any deviations from the specification?

PrizeVaults are expected to strictly comply with the ERC4626 standard.

According to the contest readme, the PrizeVaults is expected to strictly comply with the ERC4626 standard BUT prizeVault.previewDeposit() and prizeVault.previewMint() don't.

prizeVault.previewDeposit() is supposed to round down and prizeVault.previewMint() is supposed to round up.

Finally, EIP-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:

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.

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.

source: https://eips.ethereum.org/EIPS/eip-4626

prizeVault.previewDeposit() and prizeVault.previewMint() are doing 1:1 on deposits and on mint

   function previewDeposit(uint256 _assets) public pure returns (uint256) {
        // shares represent how many assets an account has deposited, so they are 1:1 on deposit
        return _assets;
    }
   function previewMint(uint256 _shares) public pure returns (uint256) {
        // shares represent how many assets an account has deposited, so they are 1:1 on mint
        return _shares;
    }

Impact

prizeVault.previewDeposit() and prizeVault.previewMint() don't comply to ERC4626 standard

Code Snippet

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L453

https://github.com/sherlock-audit/2024-05-pooltogether/blob/main/pt-v5-vault/src/PrizeVault.sol#L459

Tool used

Manual Review

Recommendation

in prizeVault.previewDeposit() round down.

in prizeVault.previewMint() round up

sherlock-admin2 commented 4 months ago

1 comment(s) were left on this issue during the judging contest.

infect3d commented:

make no sense__ there cannot be rounding error if 'func(a) equal a'

nevillehuang commented 4 months ago

Invalid per described in the documentation. shares represent how many assets an account has deposited, so they are 1:1 on deposit. Since there are no calculations here, rounding is not required