shogoki - Loss of user funds - unchecked Return of ERC20 Transfer
Opened this issue · 8 comments
shogoki
high
Loss of user funds - unchecked Return of ERC20 Transfer
Summary
Silently failing transfers can result in a partial or total loss of the users investment.
Vulnerability Detail
Some ERC20 Tokens do not revert on failure of the transfer function, but return a bool value instead. Some do not return any value. Therefore it is required to check if a value was returned, and if true, which value it is. This is not done on some places in these contracts.
The DebtIssuanceModulev2, which is required to issue or redeem tokens whenever there is a LeverageModule involved uses the invokeTransfer function of the Invoke library to transfer ERC20 Tokens from the SetToken to the user.
invokeTransfer is encoding the Calldata for the regular transfer function of ERC20 tokens and passes it together with the target (ERC20 token address) the SetTokens generic invoke function, whichin turn uses functionCallWithValue from the Openzeppelin Address Library. This method is bubbling up a possible revert of the call and returning the raw data.
The generic invoke is returning this raw data, however in invokeTransfer the return value of invoke is ignored and not used.
As some ERC20 Tokens do not revert on a failed transfer, but instead return a false bool value, the stated behaviour can lead to silently failing transfers.
This is inside the DebtIssuanceModulev2 used to:
- Transfer The "debt" (borrowed) Tokens to the user at Issuance
- Transfer back the main component Tokens (e.g. aTokens) to the user at Redemption
If such a Transfer silently fails, the funds will remain inside the setToken contract and the user has no chance to recover them.
In the issuance event the user receives the SetTokens but not the borrowed Tokens, which he has to repay when he wants to redeem the tokens. (Results in Loss of the "Debt")
In the redemption event the user repays his debt & bruns his Set Tokens, but never receives his original Tokens back. (Total Loss of investment)
Impact
Possible Loss of all/part of the Investment for the User
Code Snippet
Usage of invokeTransfer inDebtIssuanceModulev2:
invokeTransfer function ignores return value of invoke:
invoke uses functionCallwithValue and returns the raw return Data (which is ignored in this case):
Tool used
Manual Review
Recommendation
Check for the existence and value of the returned data of the Transfer call. If there is a return value, it has to be true. This could be achieved by using Openzeppelins SafeERC20 library´s safeTransfer.
Escalate for 10USDC
This should be considered a valid finding.
it backs up the existing escalation on #236
There exists a risk of loosing funds due to missing checks on the transfer calls, as described in the report.
Escalate for 10USDC
This should be considered a valid finding.
it backs up the existing escalation on #236There exists a risk of loosing funds due to missing checks on the transfer calls, as described in the report.
You've created a valid escalation for 10 USDC!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
This should definitely be a duplicate of the issue that states that the protocol does not work with tokens like USDT. This is just a superset of that issue that has the same fix.
This should definitely be a duplicate of the issue that states that the protocol does not work with tokens like USDT. This is just a superset of that issue that has the same fix.
No it should not. It should be a valid issue together with duplicates like:
#155, #236 and #280
This issue is not about USDT. It is about Tokens that do not revert on failure, which can lead to a silently failed transfer.
Result:
Medium
Has duplicates
Considering this and its duplicates a separate issue as its core issue is different from #314
Fix opened in IndexCoop/index-protocol#28
Fix looks good. Non-empty return data is now checked to accommodate tokens that return false instead of reverting