Rebasing Tokens Cause Incorrect Balance Calculations and Unexpected Behavior in safeTransferFrom Function

[howlbot-integration[bot]]

Lines of code

https://gitlab.com/thorchain/thornode/-/blob/develop/chain/ethereum/contracts/THORChain_Router.sol#L167

Vulnerability details

Impact

Tokens that rebase periodically adjust their supply, which affects balances independently of transfers. The current implementation may not account for these adjustments properly, potentially leading to incorrect balance calculations and unexpected behavior.

Proof of Concept

Rebasing tokens adjust the user's balance over time. The current code assumes that the balance change between _startBal and _endBal is only due to the transfer, which may not be true for rebasing tokens. This can lead to incorrect handling of such tokens in functions like safeTransferFrom.

Lines of Code

function safeTransferFrom(address _asset, uint _amount) internal returns(uint amount) {
    uint _startBal = iERC20(_asset).balanceOf(address(this));
    (bool success, bytes memory data) = _asset.call(abi.encodeWithSignature("transferFrom(address,address,uint256)", msg.sender, address(this), _amount));
    require(success, "Transfer failed");
    if (data.length > 0) {
        require(abi.decode(data, (bool)), "ERC20 transfer failed");
    }
    uint _endBal = iERC20(_asset).balanceOf(address(this));
    require(_endBal >= _startBal, "Transfer amount exceeds balance");
    uint transferredAmount = _endBal - _startBal;
    require(transferredAmount <= _amount, "Transferred amount exceeds requested amount");
    return transferredAmount;
}

Tools Used

Manual

Recommended Mitigation Steps

Consider implementing additional logic that can account for balance adjustments that are not directly related to transfers. One approach could be to add support for whitelisting known rebasing tokens and adjusting balance calculations accordingly.

Assessed type

ERC20

[c4-judge]

trust1995 marked the issue as not a duplicate

[c4-judge]

trust1995 marked the issue as duplicate of #85

[c4-judge]

trust1995 marked the issue as partial-50

[trust1995]

Description is quite lacking especially compared to other subs, will mark as 50%

[samuraii77]

@trust1995 Hey, I believe this issue does not constitute to a reward. From what I understand, the report mentions that the difference between the before balance and the after balance could be due to a rebasing token.

The current code assumes that the balance change between _startBal and _endBal is only due to the transfer, which may not be true for rebasing tokens.

However, what the warden mentions is impossible. He essentially says that if the _startBal is 100 and the _endBal is 150, then that could be due to a rebasing token when that's actually not true, no rebasing can occur during the function execution. It would have either occured before the function was called or after the function was called, not in the meantime.

[c4-judge]

trust1995 marked the issue as partial-25

[trust1995]

I believe the submission still deserves 25% , considering the recommended mitigation steps and the description are not wrong.

[anthonyshiks]

Hi @trust1995 the report is 100% valid and should not be partial 25 or 50 in my opinion.. here is justification and scenarios to consider: Furthermore consider this report for high severity evaluation given funds are at risk.

1. Concurrent transactions: While stETH rebasing doesn't occur mid-function, it's possible for a rebase to happen concurrently with our function execution due to the nature of blockchain transactions.

2. Scenario:

   • The safeTransferFrom function is called with stETH as the _asset.
   • At nearly the same time, the daily stETH rebase transaction is submitted.
   • Due to gas prices or miner/validator preferences, the rebase transaction gets included in a block first.
   • Our safeTransferFrom transaction follows immediately after in the same or next block.

3. Potential sequence: a. _startBal is recorded b. stETH rebase occurs (separate transaction) c. transferFrom is executed d. _endBal is recorded

4. Result: The difference between _endBal and _startBal would be larger than expected, including both the transferred amount and the rebase increase.

5. Code impact:

   uint transferredAmount = _endBal - _startBal;
   require(transferredAmount <= _amount, "Transferred amount exceeds requested amount");

   This check could potentially fail if the rebase increase plus the transfer exceeds _amount.

6. Real-world implications:

   • While rare, this scenario could occur during times of high network congestion or if the safeTransferFrom is called very close to the known rebase time.
   • It could lead to unexpected reverts or incorrect accounting of transferred amounts.

This scenario demonstrates that the broader issue of rebasing effects on balance calculations is valid. The function doesn't account for external balance changes that can occur between transaction submission and execution.

[trust1995]

@anthonyshiks we do not allow additional information to be counted towards quality of a submission after the submission deadline.

[anthonyshiks]

@trust1995 my report follows c4 guidelines and format..and issue is well highlighted in original report ..I'm just clarifying.

[anthonyshiks]

My argument is I'm seeing no reason to change to partial 50 for this reason then further change to partial 25 after @samuraii77 comment yet I have shown that the arguments used to invalidate report are false. Thanks

[samuraii77]

Hi @trust1995 the report is 100% valid and should not be partial 25 or 50 in my opinion.. here is justification and scenarios to consider: Furthermore consider this report for high severity evaluation given funds are at risk.

1. Concurrent transactions:

   While stETH rebasing doesn't occur mid-function, it's possible for a rebase to happen concurrently with our function execution due to the nature of blockchain transactions.

2. Scenario:

   • The safeTransferFrom function is called with stETH as the _asset.

   • At nearly the same time, the daily stETH rebase transaction is submitted.

   • Due to gas prices or miner/validator preferences, the rebase transaction gets included in a block first.

   • Our safeTransferFrom transaction follows immediately after in the same or next block.

3. Potential sequence:

   a. _startBal is recorded

   b. stETH rebase occurs (separate transaction)

   c. transferFrom is executed

   d. _endBal is recorded

4. Result:

   The difference between _endBal and _startBal would be larger than expected, including both the transferred amount and the rebase increase.

5. Code impact:

   
   uint transferredAmount = _endBal - _startBal;
   
   require(transferredAmount <= _amount, "Transferred amount exceeds requested amount");
   

   This check could potentially fail if the rebase increase plus the transfer exceeds _amount.

6. Real-world implications:

   • While rare, this scenario could occur during times of high network congestion or if the safeTransferFrom is called very close to the known rebase time.

   • It could lead to unexpected reverts or incorrect accounting of transferred amounts.

This scenario demonstrates that the broader issue of rebasing effects on balance calculations is valid. The function doesn't account for external balance changes that can occur between transaction submission and execution.

Do I not understand Solidity and EVM enough or is that completely impossible? How would a transaction start executing, then in the meantime another transaction would start executing and then the initial one would finish executing? That is only be possible when there is a way to hand over control over the execution (while .call is used in the code above, the transfer function definitely doesn't rebase). I don't agree that your issue deserves a partial credit at all but if that's the decision of the judge, so be it.

[anthonyshiks]

I believe if other subs are better then they should be selected for report not compared against mine and mine degraded for the comparison yet the report is valid and meets minimum standard for a sufficient quality report. Or am I mistaken?

[anthonyshiks]

@samuraii77 it seems so but that's okay we are all learning

[thebrittfactor]

For transparency, the judge has confirmed this issue is properly labeled as duplicate-85. As such, staff have updated the severity of this issue to high on their behalf.

[anthonyshiks]

@thebrittfactor noted. Does it still remain partial 25?