c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/libraries/DelegateTokenTransferHelpers.sol#L57-L59

Vulnerability details

Proof of Concept

The user delegates rights for tokens via the create function which pulls the delegateInfo.amount amount of ERC20 tokens from the user's account via the DelegateTokenTransferHelpers.pullERC20AfterCheck function. The same amount is written at the storage:

311            StorageHelpers.writeUnderlyingAmount(delegateTokenInfo, delegateTokenId, delegateInfo.amount);

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L311 Suppose there were no other deposits during the delegation period and the token was rebased such that the balance of the DelegateToken contract became less than at the start of the period. Then the user tries to withdraw tokens from the contract via withdraw (suppose the rescind function was successfully executed). The withdraw function reads the saved amount and tries transfer but throws due to insufficient balance of the DelegateToken:

371            uint256 erc20UnderlyingAmount = StorageHelpers.readUnderlyingAmount(delegateTokenInfo, delegateTokenId);
372            StorageHelpers.writeUnderlyingAmount(delegateTokenInfo, delegateTokenId, 0); // Deletes amount
373            RegistryHelpers.decrementERC20(delegateRegistry, registryHash, delegateTokenHolder, underlyingContract, erc20UnderlyingAmount, underlyingRights);
374            StorageHelpers.burnPrincipal(principalToken, principalBurnAuthorization, delegateTokenId);
375            SafeERC20.safeTransfer(IERC20(underlyingContract), msg.sender, erc20UnderlyingAmount);

Tools Used

Manual review

Recommended Mitigation Steps

Consider handling the deposits in shares instead of balances to account for rebase changes on refunds.

Assessed type

ERC20

c4-judge commented 11 months ago

GalloDaSballo marked the issue as duplicate of #257

c4-judge commented 10 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)