code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-mute/blob/main/contracts/dao/dMute.sol#L112

Vulnerability details

Proof of Concept

When redeeming, the user must iterate through all the elements of _userLock to destroy any redeemed locks. https://github.com/code-423n4/2023-03-mute/blob/main/contracts/dao/dMute.sol#L112

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();
          }
        }

However, any user can add an arbitrary number of entries to a victim's _userLocks via lockTo at a negligible expense. https://github.com/code-423n4/2023-03-mute/blob/main/contracts/dao/dMute.sol#L81

function LockTo(uint256 _amount, uint256 _lock_time, address to) public nonReentrant {
        require(IERC20(MuteToken).balanceOf(msg.sender) >= _amount, "dMute::Lock: INSUFFICIENT_BALANCE");

        //transfer tokens to this contract
        IERC20(MuteToken).transferFrom(msg.sender, address(this), _amount);

        // calculate dTokens to mint
        uint256 tokens_to_mint = timeToTokens(_amount, _lock_time);

        require(tokens_to_mint > 0, 'dMute::Lock: INSUFFICIENT_TOKENS_MINTED');

        _mint(to, tokens_to_mint);

        _userLocks[to].push(UserLockInfo(_amount, block.timestamp.add(_lock_time), tokens_to_mint));

        emit LockEvent(to, _amount, tokens_to_mint, _lock_time);
    }

Suppose the malicious user calls lockTo 100 times, with _amount = 10 (any bare minimum amount while ensuring tokens_to_mint > 0), and _lock_time = 52 weeks (maximum duration).

These 100 entries stays in _userLocks for at least 1 year (since they can't be redeemed for a year). Any time the victim redeems within this year, they must iterate through these 100 entries, wasting a significant amount of gas.

Impact

Due to the unbounded for loop where the number of iterations can be controlled by a malicious user, the victim has to pay for a lot of gas every time they call redeem. From my elementary understanding of zksync, while it uses rollups to significantly reduce the gas fee, the price for gas is still not negligible, especially in periods of low usage. Furthermore, the attacker only has to call lockTo once to add an entry, while the victim must iterate through it every time they redeem. Therefore, this setup this far from ideal.

Tools Used

Manual Review

Recommended Mitigation Steps

From my understanding, keeping the redeemed entries in _userLocks doesn't impact the functionality since the contract reverts when it hits them, and the user can select which _userLock entries to redeem.

Therefore, a potential solution is to add a bool clean parameter to the redeemTo function, and only run the for loop when clean is true.