ydlee - `updateUserDeposit` may overwrite user's existing deposit.

[sherlock-admin2]

ydlee

high

updateUserDeposit may overwrite user's existing deposit.

Summary

When updating user's deposit, if the user already has a deposit in userdetails, his existing deposit will be overridden. This will cause the user to lose his existing deposit, and subsequently receives fewer tokens upon claiming.

Vulnerability Detail

When updating user's deposit, updateUserDeposit does not check if the user already has a deposit in userdetails. It is possible that user may have existing deposits, as he can deposit multiple times through TokenSale.deposit. If the user does have a deposit, then his existing deposit will be overridden by the new deposit. In fact, the new deposit should be added to his existing deposits. This leads to user lose his existing deposits, and subsequently receives fewer tokens upon claiming, as the amount claimed is proportional to the user's total deposits.

53:    function updateUserDeposit(
54:        address[] memory _users,
55:        uint256[] memory _amount
56:    ) public onlyRole(DEFAULT_ADMIN_ROLE) {
57:        require(_users.length <= 250, "array length should be less than 250");
58:        require(_users.length == _amount.length, "array length should match");
59:        uint256 amount;
60:        for (uint256 i = 0; i < _users.length; i++) {
61:@>          userdetails[_users[i]].userDeposit = _amount[i];
62:            amount += _amount[i];
63:        }
64:        token.safeTransferFrom(distributionWallet, address(this), amount);
65:    }

https://github.com/sherlock-audit/2024-03-zap-protocol/blob/main/zap-contracts-labs/contracts/Vesting.sol#L53-L65

67:    function claim() external {
68:        address sender = msg.sender;
69:
70:        UserDetails storage s = userdetails[sender];
71:        require(s.userDeposit != 0, "No Deposit");
72:        require(s.index != vestingPoints.length, "already claimed");
73:        uint256 pctAmount;
74:        uint256 i = s.index;
75:        for (i; i <= vestingPoints.length - 1; i++) {
76:            if (block.timestamp >= vestingPoints[i][0]) {
77:@>              pctAmount += (s.userDeposit * vestingPoints[i][1]) / 10000;
78:            } else {
79:                break;
80:            }
81:        }
82:        if (pctAmount != 0) {
83:            if (address(token) == address(1)) {
84:                (bool sent, ) = payable(sender).call{value: pctAmount}("");
85:                require(sent, "Failed to send BNB to receiver");
86:            } else {
87:@>              token.safeTransfer(sender, pctAmount);
88:            }
89:            s.index = uint128(i);
90:            s.amountClaimed += pctAmount;
91:        }
92:    }

https://github.com/sherlock-audit/2024-03-zap-protocol/blob/main/zap-contracts-labs/contracts/Vesting.sol#L67-L92

Impact

User may lose his existing deposits, and subsequently receives fewer tokens upon claiming.

Code Snippet

https://github.com/sherlock-audit/2024-03-zap-protocol/blob/main/zap-contracts-labs/contracts/Vesting.sol#L53-L65

Tool used

Manual Review

Recommendation

New deposits should be added to the user's existing deposit, not overwrite it.

    function updateUserDeposit(...) {
...        
-       userdetails[_users[i]].userDeposit = _amount[i];
+       userdetails[_users[i]].userDeposit += _amount[i];
...
    }

Duplicate of #55