c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L310-L315 https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L370-L375

Vulnerability details

Impact

Rebasing tokens are tokens that have each holder's balanceOf increase over time: this includes for example AMPL and AAVE tokens.

If any of these tokens are used and deposited into escrow, their yield matured will be permanently locked, as they will be accrued to DelegateToken instead of the original owner.

Proof of Concept

Bob calls DelegateToken.create with DelegationType.ERC20, and he deposits his AAVE tokens

else if (delegateInfo.tokenType == IDelegateRegistry.DelegationType.ERC20) {
StorageHelpers.writeUnderlyingAmount(delegateTokenInfo, delegateTokenId, delegateInfo.amount);
newRegistryHash = RegistryHashes.erc20Hash(address(this), delegateInfo.rights, delegateInfo.delegateHolder, delegateInfo.tokenContract);
StorageHelpers.writeRegistryHash(delegateTokenInfo, delegateTokenId, newRegistryHash);
RegistryHelpers.incrementERC20(delegateRegistry, newRegistryHash, delegateInfo);
}

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L310-L315

Some time passes, and Bob decides to burn his principal and withdraw these tokens:

else if (delegationType == IDelegateRegistry.DelegationType.ERC20) {
    uint256 erc20UnderlyingAmount = StorageHelpers.readUnderlyingAmount(delegateTokenInfo, delegateTokenId);
    StorageHelpers.writeUnderlyingAmount(delegateTokenInfo, delegateTokenId, 0); // Deletes amount
    RegistryHelpers.decrementERC20(delegateRegistry, registryHash, delegateTokenHolder, underlyingContract, erc20UnderlyingAmount, underlyingRights);
    StorageHelpers.burnPrincipal(principalToken, principalBurnAuthorization, delegateTokenId);
    SafeERC20.safeTransfer(IERC20(underlyingContract), msg.sender, erc20UnderlyingAmount);
}

https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L370-L375

The erc20UnderlyingAmount is the same as it was when the tokens were originally deposited, but they have matured some yield over time. As such, this yield will be permanently locked inside DelegateToken, as there is no way to withdraw it.

Tools Used

Manual review

Recommended Mitigation Steps

Ideally, the owner should be able to withdraw the interest matured over time when they call the withdraw function.

Alternatively, consider fully documenting the code and the official docs, stating that rebasing tokens should NOT be used, as they are not supported.

Assessed type

ERC20

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #257