hats-finance / Tapioca--Lending-Engine--0x5bee198f5b060eecd86b299fdbea6b0c07c728dd

Other
0 stars 0 forks source link

The `Singularity::removeAsset share can become zero due to rounding down, and any user can be extracted some amount of asset` issue has not been fixed #19

Open hats-bug-reporter[bot] opened 5 months ago

hats-bug-reporter[bot] commented 5 months ago

Github username: @bauchibred Twitter username: bauchibred Submission hash (on-chain): 0xa23b1d8029983b7af9fb6728f75ee289019eb42c96930767aaa5ae826bad2c95 Severity: medium

Description: Description

Take a look at https://github.com/hats-finance/Tapioca--Lending-Engine--0x5bee198f5b060eecd86b299fdbea6b0c07c728dd/blob/8920782db6044643fd0c682f58ef37f7e59f99b1/contracts/markets/singularity/Singularity.sol#L257-L266

    function removeAsset(address from, address to, uint256 fraction)
        external
        optionNotPaused(PauseType.RemoveAsset)
        returns (uint256 share)
    {
        _accrue();
        //@audit
        share = _removeAsset(from, to, fraction);
        _allowedLend(from, share);
    }

And https://github.com/hats-finance/Tapioca--Lending-Engine--0x5bee198f5b060eecd86b299fdbea6b0c07c728dd/blob/8920782db6044643fd0c682f58ef37f7e59f99b1/contracts/markets/singularity/Singularity.sol#L426-L444

    function _removeAsset(address from, address to, uint256 fraction) private returns (uint256 share) {
        if (totalAsset.base == 0) {
            return 0;
        }
        Rebase memory _totalAsset = totalAsset;
        uint256 allShare = _totalAsset.elastic + yieldBox.toShare(assetId, totalBorrow.elastic, false);
        //@audit
        share = (fraction * allShare) / _totalAsset.base;

        _totalAsset.base -= fraction.toUint128();
        if (_totalAsset.base < 1000) revert MinLimit();

        balanceOf[from] -= fraction;
        emit Transfer(from, address(0), fraction);
        _totalAsset.elastic -= share.toUint128();
        totalAsset = _totalAsset;
        emit LogRemoveAsset(from, to, share, fraction);
        yieldBox.transfer(address(this), to, assetId, share);
    }

Now, navigate to the issue, coupling this with the @audit tag in snippets above, we can see that an attacker can use rounding down of the share of asset to zero, to remove small amounts of asset from any user, since the allowance needed in that case would be zero, though there is the _allowedLend(from, share); check, in this case it would pass considering the share is 0. Also the shares are calculated in a rounding down format, i.e share = (fraction * allShare) / _totalAsset.base; so it can aswell be 0 which makes the share 0, now the comment as the mitigation on the issue was that this was fixed in the supposed commit, however in the current commit there still seems to be no mitigation to this issue. Attack Scenario Any user can steal some amount of asset from any other user, by ensuring their calculated share rounds down to zero. see here for more info. Recommendation Consider applying the fix as suggested here, which would be to apply a requirement that share != 0 while removing the assets. Attachments

  1. Proof of Concept (PoC) File

N/A

  1. Revised Code File (Optional)

N/A

cryptotechmaker commented 5 months ago

Hey @Bauchibred, Can you open a private thread on discord? I wrote you on x (twitter) as well