Description

The DeleteOutput handler is used by the challenger to roll back nonfinalized proposals, starting at outputIndex up to the next output index, and then reset the next output index to be outputIndex.

nextOutputIndex, err := ms.GetNextOutputIndex(ctx, bridgeId)
if err != nil {
    return nil, err
}

// delete output proposals in [outputIndex, nextOutputIndex) range
for i := outputIndex; i < nextOutputIndex; i++ {
    if err := ms.DeleteOutputProposal(ctx, bridgeId, i); err != nil {
        return nil, err
    }
}

// rollback next output index to the deleted output index
if err := ms.NextOutputIndexes.Set(ctx, bridgeId, outputIndex); err != nil {
    return nil, err
}

The issue is that there is no check that the outputIndex is less than the nextOutputIndex. If a challenger submitted such an index, no outputs would be deleted, but the next output index would still be set. This would cause all future calls to ProposeOutput to fail as it tries to fetch the previous output using lastOutputProposal, err := ms.GetOutputProposal(ctx, bridgeId, outputIndex-1), which would not exist.

The challenger could also set the outputIndex to be the max value for a uint64, causing IncreaseNextOutputIndex to overflow back to zero the next time it is called.

Impact

If a challenger submits an outputIndex that is greater than the next output index, the ProposeOutput will always fail, preventing any further outputs from being proposed.

Recommendations

The outputIndex should be checked to ensure that it is less than the next output index.

Remediation