SafeERC20: add extra returndatasize check to staticInvoke() #540
Conversation
| allowTransfer_ = true; | ||
| } | ||
|
|
||
| function totalSupply() public view returns (uint256) { return totalSupply_; } |
There was a problem hiding this comment.
Now that I'm thinking of... shouldn't we provide a static version of totalSupply in SafeERC20?
There was a problem hiding this comment.
Makes sense; we didn't have a use case for it before but it doesn't hurt to include it :).
| import "../../../../lib/math/SafeMath.sol"; | ||
|
|
||
|
|
||
| contract TokenBalanceOfAllowanceReturnMissingMock { |
There was a problem hiding this comment.
WDYT about modeling this case within TokenReturnMissingMock as well?
There was a problem hiding this comment.
I was thinking about that... but what stopped me was in the TokenReturnMissingMock tests we need a working balanceOf() to check results against.
| // Else, mark call as failed | ||
| default { | ||
| success := 0 | ||
| } |
There was a problem hiding this comment.
Now that we upgraded solidity-coverage maybe we can stop using switch in favor of if-else statements, we can tackle that in a separate PR tho and migrate all at once
There was a problem hiding this comment.
Unfortunately there's no else to go along with the if (docs) :(
| } | ||
| // Else, mark call as failed | ||
| default { | ||
| success := 0 |
There was a problem hiding this comment.
Nitpick: The revert reasons in staticBalanceOf and staticAllowance denote that the call to the token reverted, which is no longer always the case with this change. I don't think we should change the revert reason but we should at least make a note for this.
There was a problem hiding this comment.
The comment in the function header says "Reverts if the call fails for some reason (should never fail)", we could just update it here.
There was a problem hiding this comment.
Modified to "(should never fail for correctly implemented ERC20s)"
For good measure :).