DOS attack to RedeemTo() and GetUnderlyingTokens(), leading to loss of funds.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-mute/blob/4d8b13add2907b17ac14627cfa04e0c3cc9a2bed/contracts/dao/dMute.sol#L135-L139

Vulnerability details

Impact

Detailed description of the impact of this finding. An attacker can launch a DOS attack to RedeemTo() and GetUnderlyingTokens() so that it will always fail for a particular account, say Bob. In this way, Bob will not be able to redeem the MuteToken locked under him. He will lose funds.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

We show how an attacker, Alice, can launch a DOS attack to RedeemTo() and GetUnderlyingTokens():

1) call LockTo(1, 52 weeks, Bob) repeatedly. As a result, the length of the array _userLocks[Bob] will be very large. The attacker only needs to spend 1wei of MuteToken.

2) When Bob calls RedeemTo(), the function will always fail due the the following for-loop as it iterates through each element in the array _userLocks[Bob]. When the length of the array is too large, this loop will run out of gas.

 for(uint256 i = _userLocks[msg.sender].length; i > 0; i--){
          UserLockInfo memory lock_info = _userLocks[msg.sender][i - 1];

          // recently redeemed lock, destroy it
          if(lock_info.time == 0){
            _userLocks[msg.sender][i - 1] = _userLocks[msg.sender][_userLocks[msg.sender].length - 1];
            _userLocks[msg.sender].pop();
          }
        }

3) GetUnderlyingTokens(Bob) will always fail due to the large length of the array _userLocks[Bob] since the function needs to iterate through each element in the array. It will run out of gas.

Tools Used

VScode

Recommended Mitigation Steps

1. Use mapping instead of array, avoiding iterating through an array who length can be very large
2. set a minimum lock value to increase the cost for attacker.

[c4-judge]

Picodes marked the issue as duplicate of #6

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 2 (Med Risk)

[c4-judge]

Picodes changed the severity to 3 (High Risk)