Math rounding in AutoRoller.sol is not ERC4626-complicant: previewWithdraw should round up.
Math rounding in AutoRoller.sol is not ERC4626-complicant: previewWithdraw should round up.\\nFinally, 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:\\nIf (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.\\nThen previewWithdraw in AutoRoller.sol should round up.\\nThe original implementation for previewWithdraw in Solmate ERC4626 is:\\n```\\n    function previewWithdraw(uint256 assets) public view virtual returns (uint256) {\\n        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.\\n\\n        return supply == 0 ? assets : assets.mulDivUp(supply, totalAssets());\\n    }\\n```\\n\\nIt is rounding up, however in the implementation of the AutoRoller.sol#previewWith is not round up.\\n```\\nfor (uint256 i = 0; i < 20;) { // 20 chosen as a safe bound for convergence from practical trials.\\n    if (guess > supply) {\\n        guess = supply;\\n    }\\n\\n    int256 answer = previewRedeem(guess.safeCastToUint()).safeCastToInt() - assets.safeCastToInt();\\n\\n    if (answer >= 0 && answer <= assets.mulWadDown(0.001e18).safeCastToInt() || (prevAnswer == answer)) { // Err on the side of overestimating shares needed. Could reduce precision for gas efficiency.\\n        break;\\n    }\\n\\n    if (guess == supply && answer < 0) revert InsufficientLiquidity();\\n\\n    int256 nextGuess = guess - (answer * (guess - prevGuess) / (answer - prevAnswer));\\n    prevGuess  = guess;\\n    prevAnswer = answer;\\n    guess      = nextGuess;\\n\\n    unchecked { ++i; }\\n}\\n\\nreturn guess.safeCastToUint() + maxError; // Buffer for pow discrepancies.\\n```\\n\\nnote the line:\\n```\\n  int256 answer = previewRedeem(guess.safeCastToUint()).safeCastToInt() - assets.safeCastToInt();\\n```\\n\\npreviewRedeem is round down.\\nand later we update guess and return guess\\n```\\n    int256 nextGuess = guess - (answer * (guess - prevGuess) / (answer - prevAnswer));\\n    prevGuess  = guess;\\n    prevAnswer = answer;\\n    guess      = nextGuess;\\n```\\n\\nand\\n```\\n return guess.safeCastToUint() + maxError; // Buffer for pow discrepancies.\\n```\\n\\nwhen calculating the the nextGuess, the code does not round up.\\n```\\nint256 nextGuess = guess - (answer * (guess - prevGuess) / (answer - prevAnswer));\\n```\\n
Round up in previewWithdraw using mulDivUp and divWadUp
Other protocols that integrate with Sense finance AutoRoller.sol might wrongly assume that the functions handle rounding as per ERC4626 expectation. Thus, it might cause some intergration problem in the future that can lead to wide range of issues for both parties.
```\\n    function previewWithdraw(uint256 assets) public view virtual returns (uint256) {\\n        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.\\n\\n        return supply == 0 ? assets : assets.mulDivUp(supply, totalAssets());\\n    }\\n```\\n