Lines of code
<https://github.com/Tapioca-DAO/tapioca-bar-audit/blob/2286f80f928f41c8bc189d0657d74ba83286c668/contracts/markets/singularity/SGLCollateral.sol#L35&gt;

Vulnerability details

Impact

The SGLCollateral contract has functionality to allow users to remove and add collateral for the Singularity market. The addCollateral function accepts a skim parameter that, if defined as true, will cause the internal _addTokens function to assert that the contract has a large enough balance for the respective token that has not been already allocated, otherwise, tokens will be transferred from the user. An important note with this functionality, is that the contract’s un-allocated funds for a respective token is calculated by subtracting the sum of the market’s total collateral shares plus the specified share amount from the contract’s yeildbox balance. Because the removeCollateral function accepts a to parameter that defines to which address the collateral should be transferred, a malicious user can double their collateral by invoking the removeCollateral function with the to parameter specified as the respective Singularity contract’s address which will then subtract the specified share amount from the totalCollateralShare state variable value while also increasing the contract’s yeildbox balance for the respective collateral by the share amount. The user can then invoke the addCollateral function with the skim parameter specified as true and the share amount specified as twice the amount that they had just specified for the removeCollateral call because the difference between the collateral shares and the contract’s yieldbox balance will be twice the amount of shares that they had just removed. This will allow the user to receive twice the amount of collateral shares that they had previously. The user can repeat these steps to completely drain the market of all it’s collateral. We believe this to be a high severity vulnerability because it places user funds and the general functionality of the protocol at risk.

Proof of Concept

If a user invokes the removeCollateral function with the to parameter specified as the Singularity contract’s address, the totalCollateralShare state variable will be reduced by the specified share amount while the contract’s contract’s yeildbox balance will simultaneously increase by the share amount. The user can then invoke the addCollateral function with the skim parameter specified as true and the share parameter as double the amount that was just removed. This will then allow them to recieve double the amount of shares that they had just removed because the difference between the contract’s yeildbox balance and the total collateral shares will be equal to, at least, twice the amount they had just removed which will allow this requirement to pass.

Tools Used

Manual Review

Recommended Mitigation Steps

It is recommended to refactor the internal _removeCollateral function so that it reverts with a custom error when the to parameter is specified as the BigBang contract’s address as shown below:

    function _removeCollateral(
        address from,
        address to,
        uint256 share
    ) internal {
        // @audit recommended mitigation
        if(to == address(this)) revert CannotRemoveToMarket();
        userCollateralShare[from] -= share;
        totalCollateralShare -= share;
        emit LogRemoveCollateral(from, to, share);
        yieldBox.transfer(address(this), to, collateralId, share);
        if (share > _yieldBoxShares[from][COLLATERAL_SIG]) {
            _yieldBoxShares[from][COLLATERAL_SIG] = 0; //accrues in time
        } else {
            _yieldBoxShares[from][COLLATERAL_SIG] -= share;
        }
    }

Assessed type

Other

The text was updated successfully, but these errors were encountered:

All reactions