You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.”
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.
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.
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.
The text was updated successfully, but these errors were encountered: