Accounting flaw in regards to `donateToSurplusBuffer()` when coupled with some credit tokens

[c4-bot-4]

Lines of code

https://github.com/code-423n4/2023-12-ethereumcreditguild/blob/2376d9af792584e3d15ec9c32578daa33bb56b43/src/governance/ProfitManager.sol#L251-L256

Vulnerability details

Proof of Concept

Take a look at ProfitManager.sol#L251-L256

function donateToSurplusBuffer(uint256 amount) external {
    CreditToken(credit).transferFrom(msg.sender, address(this), amount);
    uint256 newSurplusBuffer = surplusBuffer + amount;
    surplusBuffer = newSurplusBuffer;
    emit SurplusBufferUpdate(block.timestamp, newSurplusBuffer);
}

In this implementation, the function directly uses the amount parameter to increment the surplusBuffer, assuming that the amount of tokens specified is exactly what is transferred. However, if a token like cUSDCv3 is used as the credit, and a user specifies type(uint256).max as the amount, the actual amount transferred could be significantly less (equal to the user's balance) in cUSDCv3 case, leading to a surplus buffer accounting that does not reflect the contract's actual token balance.

Now if the surplusBuffer is 0, this newSurplusBuffer = surplusBuffer + amount would now equate to type(uint256).max, but the real balance would be a way off value, which now permanently causes a DOS to any attempt to donate to the buffer again, this is cause on the next users attempt to donate, the surplusBuffer value is already maxed out in contract, and as such would cause this to revert due to an overflow uint256 newSurplusBuffer = surplusBuffer + amount;. Now if the GUILD_SURPLUS_BUFFER_WITHDRAW governor attempts to withdraw the balance via withdrawFromSurplusBuffer() with the token's balance in the contract, this issue would permanently not be fixed, but if they pass in uint(256).max then this could be cleared, but an attacker can still just redonate 1 wei of token after governance's clearing attempt, looping us back into the issue... which is not intended logic as this leads to having to have governance always coming to bail protocol out coupled with the time delay caused by this.

Impact

The donateToSurplusBuffer function in the ProfitManager contract contains a flaw due to the function's assumption that the amount parameter accurately reflects the actual number of tokens transferred to the contract. This assumption does not hold in cases where ERC20 tokens implement non-standard behavior, such as cUSDCv3, which could lead to severe discrepancies in accounting. This flaw can potentially cause a Denial of Service (DoS) for future donations and incorrect surplus buffer accounting.

Recommended Mitigation Steps

Modify the function to accurately account for the number of tokens actually received by the contract. This can be achieved by checking the contract's token balance before and after the transferFrom call.

Assessed type

Token-Transfer

[c4-pre-sort]

0xSorryNotSorry marked the issue as sufficient quality report

[c4-pre-sort]

0xSorryNotSorry marked the issue as duplicate of #1039

[c4-judge]

Trumpero marked the issue as not a duplicate

[c4-judge]

Trumpero changed the severity to QA (Quality Assurance)

[c4-judge]

Trumpero marked the issue as grade-c

[c4-judge]

Trumpero marked the issue as grade-b