search

code-423n4 / 2023-09-delegate-findings

2 stars 1 forks source link

Rebasing tokens may be stolen or stuck in the contract #257

Closed c4-submissions closed 11 months ago

c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

Rebasing tokens, such as AMPL or rUSDY, change users' balances over time (for example to maintain the 1$ value). The DelegateToken contract saves the exact amount of underlying token at the time of delegation creation. When the PrincipalToken holder calls the withdraw() method, the contract attempts to transfer the previously saved amount. As the result, if the rebasing token's balance has changed, the holder may steal other users' deposits, receive insufficient amount of tokens or not be able to withdraw the deposit at all.

Proof of Concept

The DelegateToken#create() method escrows the specified amount of token and saves this amount in the registry.

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L311-L314

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);

Those tokens may be withdrawn by the PrincipalToken holder using the DelegateToken#withdraw() method. The following code executes the withdrawal:

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

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);

The amount to withdraw is being read from the registry and it is the exact amount of tokens that was initially escrowed. However, in case of rebasing tokens, the actual underlying balance of the token might change. Therefore the following scenarios may occur:

  1. The token's balance changed negatively and there are other user's deposits in the contract to cover for that - the withdraw() method caller will receive the original escrowed amount of token, stealing other users deposits
  2. The token's balance changed negatively and there are no other sufficient deposits to cover for that - the withdraw() method will revert as the balance is not sufficient to execute the safeTrasfer() call; the tokens will be stuck in the contract
  3. The token's balance changed positively - the withdraw() method caller will only receive the amount equal to his initial deposit; the additional balance accrued in time will be stuck in the contract

Tools Used

Manual review, Solodit

Recommended Mitigation Steps

The usage of rebasing tokens should be clearly discouraged in the documentation.

If the rebasing tokens have to be used in the protocol, they will require complex handling. DelegateToken contract should keep track of the depositor's shares in the total deposit and calculate the amount to withdraw based on the shares and the actual DelegateToken contract's balance of the underlying token.

Assessed type

ERC20

c4-judge commented 12 months ago

GalloDaSballo marked the issue as primary issue

GalloDaSballo commented 12 months ago

Slightly more detail

c4-sponsor commented 12 months ago

0xfoobar (sponsor) acknowledged

0xfoobar commented 12 months ago

Rebase tokens should be wrapped into non-rebasing tokens before being deposited, as with most other basic protocols like Uniswap.

c4-judge commented 11 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 11 months ago

Downgrading to QA and will consult with judges over how to handle this

GalloDaSballo commented 11 months ago

L

c4-judge commented 11 months ago

GalloDaSballo marked the issue as grade-c