Task Functionality completely sidestepped via autoWithdraw

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-rigor/blob/5ab7ea84a1516cb726421ef690af5bc41029f88f/contracts/Project.sol#L770

Vulnerability details

Summary

autoWithdraw will send funds to the builder, we can use this knowledge to drain all funds from Project to the builder contract. Completely sidestepping the whole Task based logic.

Impact

Through creation and deletion of tasks, leveraging autoWithdraw which will always send the funds to be builder, even when origin was the Community, a builder can cycle out all funds out of the Project Contract and transfer them to themselves.

Ultimately this breaks the trust assumptions and guarantees of the Task System, as the builder can now act as they please, the Project contract no longer holding any funds is limited.

Only aspect that diminishes impact is that the system is based on Credit (uncollateralized /undercollateralized lending), meaning the Builder wouldn't be "committing a crime" in taking ownership of all funds.

However the system invariants used to offer completely transparent accounting are now bypassed in favour of "trusting the builder".

POC

We know we can trigger autoWithdraw it by creating and allocating a task, and then reducing it's cost

            // If tasks are already allocated with old cost.
            if (tasks[_taskID].alerts[1]) {
                // If new task cost is less than old task cost.
                if (_newCost < _taskCost) {
                    // Find the difference between old - new.
                    uint256 _withdrawDifference = _taskCost - _newCost;

                    // Reduce this difference from total cost allocated.
                    // As the same task is now allocated with lesser cost.
                    totalAllocated -= _withdrawDifference;

                    // Withdraw the difference back to builder's account.
                    // As this additional amount may not be required by the project.
                    autoWithdraw(_withdrawDifference);
                } else if (totalLent - _totalAllocated >= _newCost - _taskCost) {

To funnel the funds we can:

• Create a new Task with Cost X (call addTasks)
• Allocate to it (call allocateFunds)
• changeOrder to trigger the condition if (_newCost < _taskCost) { and receive the delta of tokens

Repeat until all funds are funneled into the builder wallet.

The reason why the builder can do this is because in all functions involved:

• addTasks
• changeOrder

only the builder signature is necessary, meaning the contract is fully trusting the builder

Example Scenario

• Builder funnels the funds out
• Builder makes announcement: "Funds are safu, we'll update once we know what to do next"
• Builder follows up: "We will use twitter to post updates on the project"
• Entire system is back to being opaque, making the system pointless

Remediation Steps

Below are listed two options for mitigation

• A) Consider removing autoWithdraw (keep funds inside of project), create a separate multi-sig like way to withdraw
• B) Keep a split between funds sent by Builder and by Community, and make autoWithdraw send the funds back accordingly (may also need to re-compute total sent in Community)

[parv3213]

Users in our system are KYC'ed, whitelisted, and trusted. We are certain that they won't misuse this feature.

[jack-the-pug]

The issue makes a lot of sense to me, from the security perspective, the system should have as minimal trust as possible. The recommended remediation also makes sense.

I'm not sure about the High severity though. It's more like a Medium to me.