hats-finance / Circles-0x6ca9ca24d78af44582951825bef9eadcb210e5cf

Circles Protocol contracts
https://aboutcircles.com
GNU Affero General Public License v3.0
0 stars 0 forks source link

Incorrect Calculation of Demurraged Amount in unwrap Function #93

Open hats-bug-reporter[bot] opened 1 month ago

hats-bug-reporter[bot] commented 1 month ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0xe04a86cd932e774eadcce896b2ab6c1f7bf136ef6e54856c1ab44943ee502e4c Severity: low

Description: Description\ The unwrap function in the InflationaryCircles contract incorrectly calculates the demurraged amount when converting inflationary tokens to demurraged tokens. The function fails to account for the inflationary tokens held as debt by the avatar, potentially leading to users receiving more demurraged tokens than they should.


    function unwrap(uint256 _amount) external {
        uint256 extendedAmount = _burn(msg.sender, _amount);
        // calculate demurraged amount in extended accuracy representation
        // then discard garbage bits by shifting right
        uint256 demurragedAmount =
            convertInflationaryToDemurrageValue(extendedAmount, day(block.timestamp)) >> EXTENDED_ACCURACY_BITS;

        hub.safeTransferFrom(address(this), msg.sender, toTokenId(avatar), demurragedAmount, "");

        emit WithdrawInflationary(msg.sender, _amount, demurragedAmount);

The convertInflationaryToDemurrageValue function is called with the full extendedAmount, without subtracting the inflationary tokens balance of the avatar. This results in an inaccurate conversion, as it doesn't consider the debt held by the avatar.

Recommendation:

Modify the unwrap function to account for the avatar's inflationary token balance before converting to demurraged tokens. Here's a suggested fix:

function unwrap(uint256 _amount) external {
    uint256 extendedAmount = _burn(msg.sender, _amount);
    uint256 avatarBalance = inflationaryBalanceOf(avatar);
    uint256 adjustedAmount = extendedAmount > avatarBalance ? extendedAmount - avatarBalance : 0;

    uint256 demurragedAmount =
        convertInflationaryToDemurrageValue(adjustedAmount, day(block.timestamp)) >> EXTENDED_ACCURACY_BITS;

    hub.safeTransferFrom(address(this), msg.sender, toTokenId(avatar), demurragedAmount, "");

    emit WithdrawInflationary(msg.sender, _amount, demurragedAmount);
}
benjaminbollen commented 1 month ago

Thank you for your report on the calculation of demurraged amount in the unwrap function. After careful review, we've determined this is not an issue.

The current implementation of the unwrap function correctly calculates the demurraged amount based on the amount being unwrapped, without needing to consider the user's remaining balance. The mathematical operations used are indeed linear and behave as expected.

We appreciate your attention to the details of our demurrage calculations. While this specific concern is not applicable, your thorough examination contributes to the overall security of our system. Thank you for your participation in this security review.