TurnipBoy - UserManger#cancelVouch incomplete logic mixes up index after vouch is removed

[sherlock-admin]

TurnipBoy

high

UserManger#cancelVouch incomplete logic mixes up index after vouch is removed

Summary

When a vouch is removed it's position is swapped with the vouch in the final spot of the vouch array. The function fails to update the index of the swapped vouch. The result is that the index of the vouch no longer reflects is position inside the borrower and staker arrays.

Vulnerability Detail

    Index memory voucherIndex = voucherIndexes[borrower][staker];
    if (!voucherIndex.isSet) revert VoucherNotFound();

    Vouch memory vouch = vouchers[borrower][voucherIndex.idx];
    if (vouch.locked > 0) revert LockedStakeNonZero();

    vouchers[borrower][voucherIndex.idx] = vouchers[borrower][vouchers[borrower].length - 1];
    vouchers[borrower].pop();
    delete voucherIndexes[borrower][staker];

When a vouch is removed the function uses a swap and pop method to remove the vouch from the array. The voucherIndex contains the position of the vouch in the vouchers[borrower] array. After the swap the voucher index of the swapped vouch (which used to be at the end of the array) still stores its position as vouchers.length - 1 but it has been swapped so it's true index is that of the removed vouch.

The same issue also exists with the updated vouchees[staker] array.

Impact

The owner of the swapped voucher will not be able to cancel their vouch and the vouch array will be permanently mixed up

Code Snippet

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

Tool used

Manual Review

Recommendation

Update the index of the swapped vouch to reflect it's new position in both the vouchers[borrower] and vouchees[staker] arrays.

Duplicate of #157