PoolDeposit.sol::Wrong multiple contribution logic

[hats-bug-reporter[bot]]

Github username: @itsabinashb Twitter username: 0xAbinash Submission hash (on-chain): 0x84ea4e08780bcbe46bc6966fcd459e21ac7bc9cd21124335848fce2280122c85 Severity: high

Description: Description\ The poolDeposit() of PoolDeposit.sol contract takes an array of Contribution struct which contains address of contributor and the amount they wanna send to rabbit contract, however the as per the current logic the contributors are not sending the token to rabbit address, the token amount is taken from the msg.sender of the poolDeposit().

Let's review the iteration of contributions[]:

        for (uint i = 0; i < contributions.length; i++) {
            Contribution calldata contribution = contributions[i];
            uint256 contribAmount = contribution.amount;
            totalAmount += contribAmount;
            require(contribAmount > 0, "WRONG_AMOUNT");
            require(totalAmount >= contribAmount, "INTEGRITY_OVERFLOW_ERROR");
            uint256 depositId = allocateDepositId();
            emit Deposit(depositId, contribution.contributor, contribAmount, poolId);
        }

we can see that the contribAmount is summed up in totalAmount and an even is emited by saying that the contributor is the depositor of the contribAmount. But if we see the makeTransferFrom() call from poolDeposit():

        bool success = makeTransferFrom(msg.sender, rabbit, totalAmount);

it is visible that the total amount of token is transfered from that msg.sender of poolDeposit(). But no token is taken from contributors and sent to msg.sender so that he can transfer those token to rabbit address.

Attack Scenario\ There is actually no attack scenario, it is a logic error.

Attachments

We can fix this issue by correcting the logic of this function:

    function poolDeposit(Contribution[] calldata contributions) external {
    uint256 poolId = allocatePoolid();
    uint256 totalAmount = 0;
    for(uint256 i = 0; i < contributions.length; i++){
        Contribution calldata contribution = contributions[i];
        uint256 contribAmount = contribution.amount;
        address contributor = contribution.contributor; //@audit fetched contributor
                totalAmount += contribAmount;
                require(contribAmount > 0, "WRONG_AMOUNT");
                require(totalAmount >= contribAmount, "INTEGRITY_OVERFLOW_ERROR");
                uint256 depositId = allocateDepositId();
                emit Deposit(depositId, contributor, contribAmount, poolId);

                //@audit transfering directly from contributor to rabbit
                bool success = makeTransferFrom(contributor, rabbit, contribAmount); 
                require(success, "TRANSFER_FAILED");
    }

    require(totalAmount > 0, "WRONG_AMOUNT");
        emit PooledDeposit(poolId, totalAmount);
}

[bahurum]

poolDeposit() will be used by a third party to make multiple deposits on behalf of different contributors. It is normal that the tokens are pulled from the msg.sender (third party)