duaraghav8 / Ethlint

(Formerly Solium) Code quality & Security Linter for Solidity
https://ethlint.readthedocs.io/en/latest/
MIT License
927 stars 128 forks source link

TypeError: Cannot read property '0' of undefined #53

Closed kenji-isuntv closed 7 years ago

kenji-isuntv commented 8 years ago

When I ran the following command:

$ solium --file BlindAuction.sol

The result is:

An error occurred while running the linter on BlindAuction.sol:
TypeError: Cannot read property '0' of undefined
    at EventEmitter.<anonymous> (/usr/local/lib/node_modules/solium/lib/rules/operator-whitespace.js:24:84)
    at emitOne (events.js:96:13)
    at EventEmitter.emit (events.js:188:7)
    at EventGenerator.enterNode (/usr/local/lib/node_modules/solium/lib/utils/node-event-generator.js:25:16)
    at Controller.enter (/usr/local/lib/node_modules/solium/lib/solium.js:101:24)
    at Controller.exec (/usr/local/lib/node_modules/solium/node_modules/sol-explore/lib/traverse.js:99:21)
    at Controller.traverse (/usr/local/lib/node_modules/solium/node_modules/sol-explore/lib/traverse.js:123:17)
    at /usr/local/lib/node_modules/solium/node_modules/sol-explore/lib/traverse.js:137:17
    at Array.forEach (native)
    at Controller.traverse (/usr/local/lib/node_modules/solium/node_modules/sol-explore/lib/traverse.js:133:22)

The source code of BlindAuction.sol is the same as that in the official tutorial here:

pragma solidity ^0.4.0;

contract BlindAuction {
    struct Bid {
        bytes32 blindedBid;
        uint deposit;
    }

    address public beneficiary;
    uint public auctionStart;
    uint public biddingEnd;
    uint public revealEnd;
    bool public ended;

    mapping(address => Bid[]) public bids;

    address public highestBidder;
    uint public highestBid;

    // Allowed withdrawals of previous bids
    mapping(address => uint) pendingReturns;

    event AuctionEnded(address winner, uint highestBid);

    /// Modifiers are a convenient way to validate inputs to
    /// functions. `onlyBefore` is applied to `bid` below:
    /// The new function body is the modifier's body where
    /// `_` is replaced by the old function body.
    modifier onlyBefore(uint _time) { if (now >= _time) throw; _; }
    modifier onlyAfter(uint _time) { if (now <= _time) throw; _; }

    function BlindAuction(
        uint _biddingTime,
        uint _revealTime,
        address _beneficiary
    ) {
        beneficiary = _beneficiary;
        auctionStart = now;
        biddingEnd = now + _biddingTime;
        revealEnd = biddingEnd + _revealTime;
    }

    /// Place a blinded bid with `_blindedBid` = keccak256(value,
    /// fake, secret).
    /// The sent ether is only refunded if the bid is correctly
    /// revealed in the revealing phase. The bid is valid if the
    /// ether sent together with the bid is at least "value" and
    /// "fake" is not true. Setting "fake" to true and sending
    /// not the exact amount are ways to hide the real bid but
    /// still make the required deposit. The same address can
    /// place multiple bids.
    function bid(bytes32 _blindedBid)
        payable
        onlyBefore(biddingEnd)
    {
        bids[msg.sender].push(Bid({
            blindedBid: _blindedBid,
            deposit: msg.value
        }));
    }

    /// Reveal your blinded bids. You will get a refund for all
    /// correctly blinded invalid bids and for all bids except for
    /// the totally highest.
    function reveal(
        uint[] _values,
        bool[] _fake,
        bytes32[] _secret
    )
        onlyAfter(biddingEnd)
        onlyBefore(revealEnd)
    {
        uint length = bids[msg.sender].length;
        if (
            _values.length != length ||
            _fake.length != length ||
            _secret.length != length
        ) {
            throw;
        }

        uint refund;
        for (uint i = 0; i < length; i++) {
            var bid = bids[msg.sender][i];
            var (value, fake, secret) =
                    (_values[i], _fake[i], _secret[i]);
            if (bid.blindedBid != keccak256(value, fake, secret)) {
                // Bid was not actually revealed.
                // Do not refund deposit.
                continue;
            }
            refund += bid.deposit;
            if (!fake && bid.deposit >= value) {
                if (placeBid(msg.sender, value))
                    refund -= value;
            }
            // Make it impossible for the sender to re-claim
            // the same deposit.
            bid.blindedBid = 0;
        }
        if (!msg.sender.send(refund))
            throw;
    }

    // This is an "internal" function which means that it
    // can only be called from the contract itself (or from
    // derived contracts).
    function placeBid(address bidder, uint value) internal
            returns (bool success)
    {
        if (value <= highestBid) {
            return false;
        }
        if (highestBidder != 0) {
            // Refund the previously highest bidder.
            pendingReturns[highestBidder] += highestBid;
        }
        highestBid = value;
        highestBidder = bidder;
        return true;
    }

    /// Withdraw a bid that was overbid.
    function withdraw() returns (bool) {
        var amount = pendingReturns[msg.sender];
        if (amount > 0) {
            // It is important to set this to zero because the recipient
            // can call this function again as part of the receiving call
            // before `send` returns (see the remark above about
            // conditions -> effects -> interaction).
            pendingReturns[msg.sender] = 0;

            if (!msg.sender.send(amount)){
                // No need to call throw here, just reset the amount owing
                pendingReturns[msg.sender] = amount;
                return false;
            }
        }
        return true;
    }

    /// End the auction and send the highest bid
    /// to the beneficiary.
    function auctionEnd()
        onlyAfter(revealEnd)
    {
        if (ended)
            throw;
        AuctionEnded(highestBidder, highestBid);
        ended = true;
        // We send all the money we have, because some
        // of the refunds might have failed.
        if (!beneficiary.send(this.balance))
            throw;
    }
}
ulope commented 7 years ago

This is a minimal failing example:

pragma solidity ^0.4.4;

contract A {
    function A() {
        if (1 && 1 && 1) {

        }
    }
}

Apparently the three combined conditions are what trigger the bug. Removing the last && 1 allows solium to run normally.

ulope commented 7 years ago

After a bit more debugging the root of the problem lies in operator-whitespace.js L21-24

BinaryExpressions with more than two operands get parsed into a tree where the left node is itself a BinaryExpression. This leads to getStringBetweenNodes() returning an empty string which then breaks in L24.

However I'm not quite sure what the fix should be.

duaraghav8 commented 7 years ago

@ulope correct! I'm tracking this through tests (if you run npm test, you will see 2 operator-whitespace tests failing). To be honest, I myself haven't thought of a solution to this :P

Hopefully, I'll find time to fix this soon!

ulope commented 7 years ago

For now I'm using this truly horrendous brute force hack to work around it. This breaks whitespace violations detection in > 2 operator expressions in some cases but at least avoids the crash.

sed -i '/var strBetweenLeftAndRight/i \
if (node.left.type == "BinaryExpression") { return; }' $(npm root --global)/solium/lib/rules/operator-whitespace.js
duaraghav8 commented 7 years ago

solved in v0.2.2, thanks to @kenji-isuntv @ulope @cgewecke

Hopefully it wouldn't pose any more problems (feel free to re-open the issue otherwise) Will keep testing the fix