Lambda - UserManager.cancelVouch not updating voucherIndexes / voucheeIndexes

[sherlock-admin]

Lambda

high

UserManager.cancelVouch not updating voucherIndexes / voucheeIndexes

Summary

UserManager.cancelVouch changes indices of items within vouchers[borrower] & vouchees[staker] (because of the swap), but does not update the corresponding entry for voucherIndexes[borrower][staker] & voucheeIndexes[borrower][staker].

Vulnerability Detail

cancelVouch performs a swap within vouchers[borrower] and vouchees[staker] before removing the last item:

vouchers[borrower][voucherIndex.idx] = vouchers[borrower][vouchers[borrower].length - 1];
...
vouchees[staker][voucheeIndex.idx] = vouchees[staker][vouchees[staker].length - 1];

This means that the voucher that was previously at vouchers[borrower][vouchers[borrower].length - 1] is now at vouchers[borrower][voucherIndex.idx] (and the same for the vouchee).
However, the corresponding entry in voucherIndexes and voucheeIndexes is not updated. The function only deletes voucherIndexes[borrower][staker] and voucheeIndexes[borrower][staker] (because those entries were deleted).
But for both entries that were swapped, there is an entry voucherIndexes[borrower][some_address] and voucheeIndexes[some_other_address][staker] that now still point to their old position (which no longer exists).

Impact

This wrong logic will lead to wrong behavior wherever voucherIndexes (and voucheeIndexes) is used. We can distinguish two cases:
1.) Directly after the removal: voucherIndexes refers now to an index that no longer exists. Therefore, debtWriteOff for this staker / borrower combination will revert (meaning writing off debt is no longer possible), updateTrust will revert, getLockedStake & getVouchingAmount will return a wrong value.
2.) When a new vouch is added for a borrower: Now, the index exists again, but it refers to a completely different vouch. Therefore, all actions that access voucherIndex for this borrower / staker combination will act on the wrong vouch (for instance debtWriteOff).

After some time, it can happen that most entries in voucherIndexes refer to the wrong indices, which bricks the system completely.

Code Snippet

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/user/UserManager.sol#L589

Tool used

Manual Review

Recommendation

When doing the swap, you need to update the voucherIndexes / voucheeIndexes of the entry that corresponds to the last entry in the lists (which was swapped).

Duplicate of #157