Function _removeCredIdPerAddress() is public leading to DOS attacks when users sell shares

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/Cred.sol#L695

Vulnerability details

Impact

Attacker can DOS users from selling their shares since _removeCredIdPerAddress() is publicly exposed. This attack can performed on all the users in the system and on all of their credIds.

Proof of Concept

1. Attacker calls the _removeCredIdPerAddress() function with the credId and a user address to remove the cred id from. This removes the credId from its index. Note that the attacker can remove all credIds to scale the attack.

File: Cred.sol
701:     function _removeCredIdPerAddress(uint256 credId_, address sender_) public {
702:         // Check if the array is empty
703:         if (_credIdsPerAddress[sender_].length == 0) revert EmptyArray();
704: 
705:         // Get the index of the credId to remove
706:         uint256 indexToRemove = _credIdsPerAddressCredIdIndex[sender_][credId_];
707:         // Check if the index is valid
708:         if (indexToRemove >= _credIdsPerAddress[sender_].length) revert IndexOutofBounds();
709: 
710:         // Verify that the credId at the index matches the one we want to remove
711:         uint256 credIdToRemove = _credIdsPerAddress[sender_][indexToRemove];
712:         if (credId_ != credIdToRemove) revert WrongCredId();
713: 
714:         // Get the last element in the array
715:         uint256 lastIndex = _credIdsPerAddress[sender_].length - 1;
716:         uint256 lastCredId = _credIdsPerAddress[sender_][lastIndex];
717:         // Move the last element to the position of the element we're removing
718:         _credIdsPerAddress[sender_][indexToRemove] = lastCredId;
719: 
720:         // Update the index of the moved element, if it's not the one we're removing
721:         if (indexToRemove < lastIndex) {
722:             _credIdsPerAddressCredIdIndex[sender_][lastCredId] = indexToRemove;
723:         }
724: 
725:         // Remove the last element (which is now a duplicate)
726:         _credIdsPerAddress[sender_].pop();
727: 
728:         // Remove the index mapping for the removed credId
729:         delete _credIdsPerAddressCredIdIndex[sender_][credIdToRemove];
730: 
731:         // Decrement the array length counter, if it's greater than 0
732:         if (_credIdsPerAddressArrLength[sender_] > 0) {
733:             _credIdsPerAddressArrLength[sender_]--; 
734:         }
735:     }

2. When user calls function sellShareCred() on Cred.sol to sell all pending shares, the call routes as follows: _handleTrade() => _updateCuratorBalance => _removeCredIdPerAddress().

3. In the _removeCredIdPerAddress() in the code snippet above, if the attacker removes all credIds, we revert due to Line 703. If the user still has some credIds in the array but the credId being sold completely is not in the array due the attack, we revert due to Line 712 since the credId at index 0 would not match the credId that has already been removed.

Note: Although the user could add the credId back using _addCredIdPerAddress(), the attacker can still perform the attack again. Normal users won't be having the technical capabilities to make batched calls i.e. _addCredIdPerAddress() and sell shares in one batch tx, which could solve this issue in production, but it cannot be relied upon.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider making both the _removeCredIdPerAddress() and _addCredIdPerAddress() functions internal.

Assessed type

DoS