Frankencoin - Diana's results

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L100

Vulnerability details

Impact

Wrong value is returned for function reduceLimitForClone().

As per the natspec comment, reduceLimitForClone() should return the limit for the clone which is calculated as limit -= reduction + _minimum;, however the function wrongly returns the value reduction + _minimum

Due to this, when clonePosition() would be executed in MintingHub.sol, the wrong value will be set for limit at MintingHub.sol#L126:

uint256 limit = existing.reduceLimitForClone(_initialMint);

Proof of Concept

As per the natspec comment at Position.sol#L95, the function should return limit for the clone, which is calculated at Position.sol#L99

limit -= reduction + _minimum;

The new limit value should now be:

limit = limit - reduction + _minimum

However, the function wrongly returns the value of reduction + _minimum instead of limit at Position.sol#L100

return reduction + _minimum;

Due to this, when clonePosition() would be executed in MintingHub.sol#L126, the wrong value will be set for limit:

uint256 limit = existing.reduceLimitForClone(_initialMint);

Tools Used

Manual review

Recommended Mitigation Steps

function reduceLimitForClone(uint256 _minimum) external noChallenge noCooldown alive onlyHub returns (uint256) {
        uint256 reduction = (limit - minted - _minimum)/2; // this will fail with an underflow if minimum is too high
        limit -= reduction + _minimum;
-        return reduction + _minimum;
+        return limit;
    }