`Basket.sol#publishNewIndex()` Lack of input validation may cause fund loss to anyone who bonds an auction

[code423n4]

Handle

WatchPug

Vulnerability details

According to the newRatio formula in settleAuction(), the maximum value of newRatio is factory.auctionMultiplier() * basket.ibRatio().

However, since there is no validation for the value of minIbRatio when setting it, if the publisher publishes a newIndex with minIbRatio larger than factory.auctionMultiplier() * basket.ibRatio(), settleAuction() will always fail.

https://github.com/code-423n4/2021-12-defiprotocol/blob/205d3766044171e325df6a8bf2e79b37856eece1/contracts/contracts/Auction.sol#L97-L99

    uint256 a = factory.auctionMultiplier() * basket.ibRatio();
    uint256 b = (bondBlock - auctionStart) * BASE / factory.auctionDecrement();
    uint256 newRatio = a - b;

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();
    }
}

PoC

• bondPercentDiv = 400
• basketToken.totalSupply = 40,000
• factory.auctionMultiplier = 2
• factory.auctionDecrement = 10,000
• basket.ibRatio = 1e18

1. The publisher called publishNewIndex() with _minIbRatio = 2e18
2. Alice call bondForRebalance() just after 1 block, paid 100 basketToken
3. Alice tries to settleAuction(), it will always fail because newRatio < minIbRatio
   • a = 2 * 1e18
   • b = 0.0001 * 1e18
   • newRatio = 1.9999 * 1e18;
4. Anyone can call bondBurn() after 24 hrs, and Alice's 100 basketToken will be burned.

Recommendation

Change to:

function publishNewIndex(address[] memory _tokens, uint256[] memory _weights, uint256 _minIbRatio) onlyPublisher public override {
        validateWeights(_tokens, _weights);
        require(_minIbRatio < factory.auctionMultiplier() * ibRatio, "!_minIbRatio");

        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();
        }
    }

[frank-beard]

while this could technically happen, it should be up to the auction rebalancer to make sure they can actually settle the auction whether that's how much capital is required, and possible issues with the new weights. i would consider this a low risk issue.

[0xleastwood]

I agree, I think it is expected that the auction rebalancer will check minIbRatio before calling bondForRebalance(). However, I actually think this is a duplicate of #53 as it also describes how an auction rebalancer can have their tokens burnt by a malicious publisher.