Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: calls-loop does not report the context of the loop #1468

Open
itinance opened this issue Nov 14, 2022 · 3 comments
Open

[Bug]: calls-loop does not report the context of the loop #1468

itinance opened this issue Nov 14, 2022 · 3 comments
Labels
bug Something isn't working
Milestone

Comments

@itinance
Copy link

Describe the issue:

Slither reports "has external calls inside a loop" although the corresponding function only contains an if -statement.

Code example to reproduce the issue:

Example 1:

    function _hasRole(uint256 tokenId, address account) internal view virtual returns (bool) {
        if (_permissions.balanceOf(account, tokenId) > 0) {
            return true;
        }

        return false;
    }

Example 2:

    function _doSafeTransferAcceptanceCheck(
        address operator,
        address from,
        address to,
        uint256 id,
        uint256 amount,
        bytes memory data
    ) private {
        if (to.isContract()) {
            try IERC1155Receiver(to).onERC1155Received(operator, from, id, amount, data) returns (bytes4 response) {
                if (response != IERC1155Receiver.onERC1155Received.selector) {
                    revert("ERC1155: ERC1155Receiver rejected tokens");
                }
            } catch Error(string memory reason) {
                revert(reason);
            } catch {
                revert("ERC1155: transfer to non ERC1155Receiver implementer");
            }
        }
    }

Version:

0.8.3

Relevant log output:

PermissionsAware._hasRole(uint256,address) (contracts/permissions/PermissionsAware.sol#44-50) has external calls inside a loop: _permissions.balanceOf(account,tokenId) > 0 (contracts/permissions/PermissionsAware.sol#45)
ERC1155._doSafeTransferAcceptanceCheck(address,address,address,uint256,uint256,bytes) (node_modules/@openzeppelin/contracts/token/ERC1155/ERC1155.sol#467-486) has external calls inside a loop: IERC1155Receiver(to).onERC1155Received(operator,from,id,amount,data) (node_modules/@openzeppelin/contracts/token/ERC1155/ERC1155.sol#476-484)
Reference: https://github.com/crytic/slither/wiki/Detector-Documentation/#calls-inside-a-loop
@itinance itinance added the bug-candidate Bugs reports that are not yet confirmed label Nov 14, 2022
@0xalpharush
Copy link
Contributor

Are _hasRole or _doSafeTransferAcceptanceCheck called within loops anywhere in the codebase?

@itinance
Copy link
Author

Are _hasRole or _doSafeTransferAcceptanceCheck called within loops anywhere in the codebase?

No, definitely no loops. But calls like that:

if (!_hasRole(TOKEN_ROLE_ISSUER, from) && !_hasRole(TOKEN_ROLE_ADMIN, from)) {

Could it be that this counts as "loop" because _hasRole was called twice?

The second example with _doSafeTransferAcceptanceCheck is from OpenZeppelin (ERC1155-contract). I couldn't find a loop also in their repository.

@PaulRBerg
Copy link
Contributor

PaulRBerg commented Mar 18, 2023

Here's another example that triggers this bug:

function _cancel(uint256 streamId) internal override onlySenderOrRecipient(streamId) {
    // beginning of function clipped
    // ...
    if (msg.sender == sender) {
        if (recipient.code.length > 0) {
            try ISablierV2LockupRecipient(recipient).onStreamCanceled({
                streamId: streamId,
                senderAmount: senderAmount,
                recipientAmount: recipientAmount
            }) { } catch { }
        }
    }
    else {
        if (sender.code.length > 0) {
            try ISablierV2LockupSender(sender).onStreamCanceled({
                streamId: streamId,
                senderAmount: senderAmount,
                recipientAmount: recipientAmount
            }) { } catch { }
        }
    }
}

_cancel is called by other functions in the code base, some of which do contain loops. But those loop-containing functions are not reported.

@0xalpharush 0xalpharush added bug Something isn't working and removed bug-candidate Bugs reports that are not yet confirmed labels Mar 18, 2023
@0xalpharush 0xalpharush changed the title [Bug-Candidate]: Wrong detection of a loop in an if-statement [Bug]: Wrong detection of a loop in an if-statement Mar 18, 2023
@0xalpharush 0xalpharush changed the title [Bug]: Wrong detection of a loop in an if-statement [Bug]: calls-loop does not report the context of the loop Apr 18, 2023
@0xalpharush 0xalpharush added this to the 0.9.4 milestone Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants