`NewIndexSubmitted` event is not emitted in some case

[code423n4]

Handle

WatchPug

Vulnerability details

Based on the context, the event NewIndexSubmitted should be emitted every time a new pending index is set.

However, in the current implementation of publishNewIndex(), NewIndexSubmitted is not emitted when at around L233, when an auction is ongoing and not bonded yet.

Considering that as a basketToken holder, getting to know when and how the index is going to be updated is crucial, we mark this issue as a Medium.

We believe that essentially the protocol has made a promise to the basketToken holders that they will get notified 24 hours before any changes happen to their holdings.

Not sending that notification should be considered a major issue, which breaks that promise.

A malicious publisher may take advantage of this and rug the basketToken holders by setting an unfair new index.

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Basket.sol#L216-L244

function publishNewIndex(address[] memory _tokens, uint256[] memory _weights, uint256 _minIbRatio) onlyPublisher public override {
    validateWeights(_tokens, _weights);

    if (pendingWeights.pending) {
        require(block.timestamp >= pendingWeights.timestamp + TIMELOCK_DURATION);
        if (auction.auctionOngoing() == false) {
            auction.startAuction();

            emit PublishedNewIndex(publisher);
        } else if (auction.hasBonded()) {

        } else {
            auction.killAuction();

            pendingWeights.tokens = _tokens;
            pendingWeights.weights = _weights;
            pendingWeights.timestamp = block.timestamp;
            pendingWeights.minIbRatio = _minIbRatio;
        }
    } else {
        pendingWeights.pending = true;
        pendingWeights.tokens = _tokens;
        pendingWeights.weights = _weights;
        pendingWeights.timestamp = block.timestamp;
        pendingWeights.minIbRatio = _minIbRatio;

        emit NewIndexSubmitted();
    }
}

Recommendation

Change to:

function publishNewIndex(address[] memory _tokens, uint256[] memory _weights, uint256 _minIbRatio) onlyPublisher public override {
    validateWeights(_tokens, _weights);

    if (pendingWeights.pending) {
        require(block.timestamp >= pendingWeights.timestamp + TIMELOCK_DURATION);
        if (auction.auctionOngoing() == false) {
            auction.startAuction();

            emit PublishedNewIndex(publisher);
        } else if (auction.hasBonded()) {

        } else {
            auction.killAuction();

            pendingWeights.tokens = _tokens;
            pendingWeights.weights = _weights;
            pendingWeights.timestamp = block.timestamp;
            pendingWeights.minIbRatio = _minIbRatio;

            emit NewIndexSubmitted();
        }
    } else {
        pendingWeights.pending = true;
        pendingWeights.tokens = _tokens;
        pendingWeights.weights = _weights;
        pendingWeights.timestamp = block.timestamp;
        pendingWeights.minIbRatio = _minIbRatio;

        emit NewIndexSubmitted();
    }
}

[frank-beard]

would consider this a non-critical/low risk issue

[0xleastwood]

I believe this qualifies as a non-critical issue as events not being emitted isn't a direct security risk.