bitsurfer - The return data of the `Invoke` function is not properly verified when using the Transfer and Approve ERC20 functions.
Closed this issue · 6 comments
bitsurfer
medium
The return data of the Invoke function is not properly verified when using the Transfer and Approve ERC20 functions.
Summary
The return data of the Invoke function is not properly verified when using the Transfer and Approve ERC20 functions.
Vulnerability Detail
Index Coop has a custom Invoke invoke() function specific to encapsulate call for some ERC20 function like Transfer and Approve.
The issue here is the return of that call doesn't check or filter again if the call to ERC20 function is success or failed.
In the ERC20 standard, it is crucial to validate the return values of transfer and approve functions to ensure the transaction was executed successfully and to handle any potential errors or exceptions that may occur.
File: Invoke.sol
46: function invokeApprove(
47: ISetToken _setToken,
48: address _token,
49: address _spender,
50: uint256 _quantity
51: )
52: internal
53: {
54: bytes memory callData = abi.encodeWithSignature("approve(address,uint256)", _spender, _quantity);
55: _setToken.invoke(_token, 0, callData);
56: }
...
66: function invokeTransfer(
67: ISetToken _setToken,
68: address _token,
69: address _to,
70: uint256 _quantity
71: )
72: internal
73: {
74: if (_quantity > 0) {
75: bytes memory callData = abi.encodeWithSignature("transfer(address,uint256)", _to, _quantity);
76: _setToken.invoke(_token, 0, callData);
77: }
78: }
File: SetToken.sol
197: function invoke(
198: address _target,
199: uint256 _value,
200: bytes calldata _data
201: )
202: external
203: onlyModule
204: whenLockedOnlyLocker
205: returns (bytes memory _returnValue)
206: {
207: _returnValue = _target.functionCallWithValue(_data, _value);
208:
209: emit Invoked(_target, _value, _data, _returnValue);
210:
211: return _returnValue;
212: }Failure to appropriately check the return values of token transfers or approvals within the invoke() function may result in potential errors or issues going unnoticed. This can lead to incorrect assumptions about the success of these operations, potentially compromising the integrity of the token transfers or approvals.
Impact
potential errors or issues going unnoticed, leading to incorrect assumptions about the success of token transfers or approvals.
Code Snippet
See above
Tool used
Manual Review
Recommendation
Use OpenZeppelin library for this issue, safeTransfer & safeApprove or customize it to include / filter the safe return data
require(returndata.length == 0 ||
abi.decode(returndata, bool), "SafeERC20: ERC20 operation did not succeed");Duplicate of #169
Escalate for 10 USDC
This issue wrongly dups with #314, which is only limited to USDT and Approve issue.
Please reconsider again this issue, as this issue is about return data checks in the Invoke library.This issue is similar to:
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.
Most of the issues regarding broken erc20 implementations that include approvals, unchecked transfers etc. are mostly different mini issues in differents part of tbe code that all of them describe the same. Current implementation does not work with usdt and approvals are not done correctly. Can't open single issue for all of them, and despite your issue being more elaborated to actual impact, it has the same fix than any of the others. Approve to 0 first. Should keep the duplicate
Quoting:
... it has the same fix than any of the others. Approve to 0 first. Should keep the duplicate
This issue will not just fixed by doing Approve 0. It also mention about transfer issue function, just like the other duplicates I mentioned above.
according to: https://docs.sherlock.xyz/audits/judging/judging#duplication-rules
In case the same vulnerability appears across multiple places in different contracts, they can be considered duplicates.
The exception to this would be if underlying code implementations, impact, and the fixes are different, then they can be treated separately.
since the fixes are different, I believe it can be treated separately.
Moreover, this might be the perfect example of Identifies the core issue section
Identifies the core issue: In case of issues that have a large number of duplicates, Issues that identify the core issue and show valid loss of funds should be grouped and the others would be downgraded accordingly.
Result:
Medium
Duplicate of #169