Ignored return values may lead to undefined behavior

[code423n4]

Handle

0xRajeev

Vulnerability details

Impact

The _depositInVault() returns the value returned from its call to the Yearn vault’s deposit() function. However, the return value is ignored at both call sites in supplyTokenTo() and sponsor().

Impact: It is unclear what the intended usage is and how, if any, the return value should be checked. This should perhaps check how much of the full balance was indeed deposited/rejected in the vault by comparing the return value of issued vault shares as commented: “The actual amount of shares that were received for the deposited tokens” because "if deposit limit is reached, tokens will remain in the Yield Source and they will be queued for retries in subsequent deposits.”

Proof of Concept

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/yield-source/YearnV2YieldSource.sol#L175

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/yield-source/YearnV2YieldSource.sol#L129

https://github.com/code-423n4/2021-06-pooltogether/blob/85f8d044e7e46b7a3c64465dcd5dffa9d70e4a3e/contracts/yield-source/YearnV2YieldSource.sol#L158

Tools Used

Manual Analysis

Recommended Mitigation Steps

Check return value appropriately or if not, document why this is not necessary.

[asselstine]

Regardless of how much of the deposit made it into the underlying vault, the depositor will hold the correct number shares. It doesn't matter if only a portion of the funds made it into the vault.

[dmvt]

I think this is a reasonable finding by the warden. If the return value isn't needed, it should be removed or at least documented that it's there for no reason. If there is a reason to have the return value, the return value should be considered by the calling functions.