Checking for zero amounts can avoid unnecessary further execution flow

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

Functions that accept an amount from users can avoid unnecessary execution flow by checking for zero value (accidental or intentional) in which case further execution flow can be entirely skipped in many cases. This can save gas from external calls (that may rely on non-zero values) or other expensive operations that would have no useful side-effects from the zero amount value anyway.

Proof of Concept

https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L186-L187

https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L197-L198

https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L208

https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L234

https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L278

https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L296

https://github.com/pooltogether/swappable-yield-source/blob/89cf66a3e3f8df24a082e1cd0a0e80d08953049c/contracts/SwappableYieldSource.sol#L323

https://github.com/pooltogether/pooltogether-mstable/blob/0bcbd363936fadf5830e9c48392415695896ddb5/contracts/yield-source/MStableYieldSource.sol#L82

https://github.com/pooltogether/pooltogether-mstable/blob/0bcbd363936fadf5830e9c48392415695896ddb5/contracts/yield-source/MStableYieldSource.sol#L94

Tools Used

Manual Analysis

Recommended Mitigation Steps

Add input validation for zero amount and put all the logic within a conditional on that predicate.

E.g.:

function _mintShares(uint256 mintAmount, address to) internal { if (mintAmount > 0) { uint256 shares = _tokenToShares(mintAmount); require(shares > 0, “SwappableYieldSource/shares-gt-zero"); _mint(to, shares); } }

[PierrickGT]

These additions seem superfluous and all these require will complexify our code. In some cases, we can always enforce it in the front-end if we want to. For the mint function in the SwappableYieldSource, we already have a condition to verify that the user can't mint if shares are not superior to 0.

[0xean]

closing issue as these zero checks add gas costs in most normal scenarios and don't pose any real risk