search

code-423n4 / 2023-08-pooltogether-mitigation-findings

0 stars 0 forks source link

M-04 MitigationConfirmed #7

Open code423n4 opened 1 year ago

code423n4 commented 1 year ago

Lines of code

Vulnerability details

Comments

The previous implementation did not validate that maxMint was not exceeded on a deposit. It was therefore possible that the maxMint could be exceeded on a new deposit if the Vault was sufficiently under-collateralized.

Mitigation

The updated implementation now does 2 additional checks. Firstly it verifies that the amount of assets/shares (converted to shares) doesn’t exceed type(uint96).max since this is the type used to store balances in the TwabController. Secondly it also checks maxMint and maxDeposit of the underlying yield vault and then caps the mint/deposit at the minimum of the two possible values. This change ensures the contract is now properly ERC-4626 compliant and resolves the original issue. This mitigation overlaps significantly with M-23.

Conclusion

LGTM

c4-judge commented 1 year ago

Picodes marked the issue as satisfactory

Picodes commented 1 year ago

@djb15 just a note for future contests that although your writing is great, the "LGTM" doesn't push to choose your issues for the final report

djb15 commented 1 year ago

Thanks for the advise @Picodes, much appreciated. It's my first mitigation review so I was just following some of the example "Mitigation confirmed" reports from the C4 Notion. Will be more expressive next time round