Audit: Steal of user assets

[KupiaSecAdmin]

Severity

High

Details

We believe the MCV1_Wrapper is supposed to work as a wrapper of MintClubBond and MCV1 users are assumed to work with this wrapper to buy and sell MCV1 bonds.
For example, users can buy bonds by calling the function MCV1_Wrapper::mint().

MCV1_Wrapper.sol
35:     function mint(address token, uint256 tokensToMint, uint256 maxReserveAmount) external {
36:         (uint256 reserveAmount, uint256 royalty) = getReserveForToken(token, tokensToMint);
37:         uint256 reserveRequired = reserveAmount + royalty;
38: 
39:         if (maxReserveAmount < reserveRequired) revert MCV1_Wrapper__SlippageLimitExceeded();
40: 
41:         BOND.buy(token, reserveRequired, tokensToMint, BENEFICIARY);
42:     }

But the MCV1_Wrapper becomes the msgSender() when the wrapper calls buy and sell functions of MintClubBond. (L41 above)
MintClubBond::buy() is implemented as below.

function buy(address tokenAddress, uint256 reserveAmount, uint256 minReward, address beneficiary) public {
        (uint256 rewardTokens, uint256 taxAmount) = getMintReward(tokenAddress, reserveAmount);
        require(rewardTokens >= minReward, "SLIPPAGE_LIMIT_EXCEEDED");

        // Transfer reserve tokens
        require(RESERVE_TOKEN.transferFrom(_msgSender(), address(this), reserveAmount - taxAmount), "RESERVE_TOKEN_TRANSFER_FAILED");
        reserveBalance[tokenAddress] += (reserveAmount - taxAmount);

        // Mint reward tokens to the buyer
        MintClubToken(tokenAddress).mint(_msgSender(), rewardTokens);

        // Pay tax to the beneficiary / Send to the default beneficiary if not set (or abused)
        address actualBeneficiary = beneficiary;
        if (beneficiary == address(0) || beneficiary == _msgSender()) {
            actualBeneficiary = defaultBeneficiary;
        }
        RESERVE_TOKEN.transferFrom(_msgSender(), actualBeneficiary, taxAmount);

        emit Buy(tokenAddress, _msgSender(), rewardTokens, reserveAmount, actualBeneficiary, taxAmount);
    }

As we can see in the above implementation, the function will pulls the RESERVE_TOKEN from the _msgSender().
This means the wrapper contract is assumed to have the required RESERVE_TOKEN beforehand.
We guess the protocol assumed the users will send the RESERVE_TOKEN before calling the wrapper's buy function.
But because there is no validation, an attacker can monitor the mempool and frontrun to call this function before the normal user and it will send the bought bonds to the attacker instead of the user.

[sydneyitguy]

Oh, you're right! I haven't tested the MCV1_Wrapper function yet, and it seems all those writing functions are implemented incorrectly. I will fix this tomorrow.

Thanks!

[KupiaSecAdmin]

Some statements might be not accurate. For example, maybe the bought bonds will be sent to the wrapper contract and locked there, not minted to the attacker. We suggest building an integration test suite (Foundry strongly recommended) to avoid this kind of integration mistakes.

[sydneyitguy]

We've decided to remove those writing functions from the wrapper because we can just call the V1 bond contract directly. Using the wrapper function forwarding those assets in between may just add gas fees.

[KupiaSecAdmin]

eth:0x2263C972ad2e80198F219BAa763B2b7D19ce4C09

Feel free to reach out to kupia.io for mitigation review.