obront - cancelVouch breaks voucher and vouchee index tracking, creating opportunity for user theft

[sherlock-admin]

obront

high

cancelVouch breaks voucher and vouchee index tracking, creating opportunity for user theft

Summary

The cancelVouch() function creates mismatched voucherIndexes and voucheeIndexes by moving the final element in the array into the deleted element's place, but not updating the index tracking mapping.

As a result, a user can create a situation where their voucherIndex is actually occupied by another user's vouch, which could be used to steal from them.

Vulnerability Detail

The cancelVouch() function uses the swap-and-pop technique to remove a vouch by replacing it with the final vouch in the array and popping the final value off the array. This is implemented correctly, however, when this action is performed, the index of the final vouch in the array is not updated.

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

In this piece of code, the final element in the vouchers[borrower] array is moved into the place of the element to be deleted, and then the final element in the array is removed.

However, after this action is taken, the value of that final moved value in voucherIndexes[borrower][finalMovedValue] remains as the final element in the array.

This is true for both the voucherIndex and the voucheeIndex.

This can be used for a user to steal from another user. Consider the following example:

• I vouch for a user from one account.
• Later, I vouch for a user from another account, making this account the final element in the vouchers[borrower] array.
• I cancel my first vouch. This moves my second vouch to the earlier position in the array, but keeps my index as the final index in the array, which is then shortened by one.
• The next person to vouch now has the same index as me.
• I can now call updateTrust() for my own account. It will check my index, return the final element of the array, and use that index to retrieve the new user's vouch. I then have free reign to update their vouch to a large amount.
• With this large amount, I can borrow, default on the debt, and have them incur the cost.

This is just one example, but there are countless possibilities (intentional and unintentional) that can happen by having one user's index represent another user's vouch.

Impact

Opens the door to intentional hacks that steal user funds, or accidental behaviors with similar consequences.

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 each of the swap-and-pops to set the moved element's index to its new position:

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

The same patterns should be followed for the vouchee.

Duplicate of #157