underflow in Lock.release() can lock up funds

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Lock.sol#L73
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Lock.sol#L84-L92
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/Lock.sol#L103

Vulnerability details

Impact

An underflow in Lock.release() can prevent a lock from being released. That means funds are locked up.

This issue directly affects user funds and doesn't depend on any external requirements. Someone just has to extend their lock and add some funds to it for this be an issue somewhere down the road.

Proof of Concept

When an existing lock is extended, the new amount of assets added to the lock is not included in the totalLocked state variable. When the user tries to release their lock, lockAmount is bigger than totalLocked[asset] causing an underflow:

    function lock(
        address _asset,
        uint _amount,
        uint _period
    ) public {
        require(_period <= maxPeriod, "MAX PERIOD");
        require(_period >= minPeriod, "MIN PERIOD");
        require(allowedAssets[_asset], "!asset");

        claimGovFees();

        IERC20(_asset).transferFrom(msg.sender, address(this), _amount);
        totalLocked[_asset] += _amount;
        
        bondNFT.createLock( _asset, _amount, _period, msg.sender);
    }

    /**
     * @notice Reset the lock time and extend the period and/or token amount
     * @param _id Bond id being extended
     * @param _amount tigAsset amount being added
     * @param _period number of days being added
     */
    function extendLock(
        uint _id,
        uint _amount,
        uint _period
    ) public {
        address _asset = claim(_id);
        IERC20(_asset).transferFrom(msg.sender, address(this), _amount);
        bondNFT.extendLock(_id, _asset, _amount, _period, msg.sender);
    }

    /**
     * @notice Release the bond once it's expired
     * @param _id Bond id being released
     */
    function release(
        uint _id
    ) public {
        claimGovFees();
        (uint amount, uint lockAmount, address asset, address _owner) = bondNFT.release(_id, msg.sender);
        totalLocked[asset] -= lockAmount;
        IERC20(asset).transfer(_owner, amount);
    }

Here's a test showcasing the issue:

// 09.Bond.js

    it.only("causes an underflow", async function() {
      await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("100"));
      await lock.connect(user).lock(StableToken.address, ethers.utils.parseEther("100"), 10);
      await stabletoken.connect(owner).mintFor(user.address, ethers.utils.parseEther("100"));
      await lock.connect(user).extendLock(1, ethers.utils.parseEther("100"), 0);

      await network.provider.send("evm_increaseTime", [432000 * 3]); // Skip 5 days
      await network.provider.send("evm_mine");

      await lock.connect(user).release(1);
    });

In most cases, this will only affect the very last user to release their lock. But, a whale who holds a majority of the tokens could encounter the same issue as well. It depends on the way to lock was set up.

To unlock these funds, someone else has to create a lock to increase the totalLocked value. There's no way to get all the tokens out. Some amount will always be lost.

Tools Used

none

Recommended Mitigation Steps

In extendLock() add the new locked funds to totalLocked.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #23

[c4-judge]

GalloDaSballo marked the issue as satisfactory