Beanstalk: The Finale
Beanstalk
DeFiHardhatFoundry
250,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

`Internalizer::_transfer()` unsafe data type conversion may cause serious problems

joicygiore

Summary

Internalizer::_transfer() unsafe data type conversion may cause serious problems

Vulnerability Details

function _transfer(
address from,
address to,
uint256 id,
uint256 amount
) internal virtual override {
@> uint128 _amount = uint128(amount);
if (from != address(0)) {
uint128 fromBalance = _balances[id][from].amount;
require(uint256(fromBalance) >= amount, "ERC1155: insufficient balance for transfer");
// Because we know fromBalance >= amount, we know amount < type(uint128).max
_balances[id][from].amount = fromBalance - _amount;
}
_balances[id][to].amount = _balances[id][to].amount.add(_amount);
}

Type conversion truncates the high bits that exceed the bit width of the target type. Therefore, when you convert a uint256 type maximum value (type(uint256).max) to a uint128 type, the converted value is actually the maximum value of the uint128 type (type(uint128).max).
When you convert type(uint256).max to a uint128 type, only the lower 128 bits are retained and the upper 128 bits are truncated. Therefore, the converted value is the maximum value of uint128 and there will be no error message.
The maximum value of the uint256 type is 2^256 - 1, which is 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF.
The maximum value of the uint128 type is 2^128 - 1, which is 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF.
Since Internalizer::_transfer() is used to override Fertilizer1155::_transfer(), Internalizer::_transfer() will be executed when Fertilizer1155::safeTransferFrom(), Fertilizer1155::safeBatchTransferFrom() and other methods are called.
Let's take Fertilizer1155::safeTransferFrom() as an example:

function safeTransferFrom(
address from,
address to,
uint256 id,
uint256 amount,
bytes memory data
) public virtual override {
require(to != address(0), "ERC1155: transfer to the zero address");
require(
from == _msgSender() || isApprovedForAll(from, _msgSender()),
"ERC1155: caller is not owner nor approved"
);
​
​
address operator = _msgSender();
​
​
_beforeTokenTransfer(
operator,
from,
to,
__asSingletonArray(id),
__asSingletonArray(amount),
data
);
​
​
@> _transfer(from, to, id, amount);
​
​
@> emit TransferSingle(operator, from, to, id, amount);
​
​
__doSafeTransferAcceptanceCheck(operator, from, to, id, amount, data);
}

Potential issues

  1. Inaccurate event logs: The transfer amounts recorded by emit TransferBatch(operator, from, to, ids, amounts); do not match the actual amounts transferred. This can cause inaccurate states recorded by external systems that rely on these event logs for state tracking or auditing (such as blockchain browsers, indexers).

  2. Inconsistent state updates: Due to the truncation of amounts, the actual updated amounts in _balances are less than expected, which may cause the internal state of the contract to be inconsistent with the external expected state, further leading to fund mismatches or account balance errors.

  3. Possible security vulnerabilities: Attackers may exploit this to bypass certain checks or restrictions by passing an amount exceeding type(uint128).max, especially when other logic depends on these amounts.

Specific impact

When calling _transfer in Fertilizer1155::safeTransferFrom(), if amount exceeds type(uint128).max, the following may happen:

  1. For the from account:

uint256 fromBalance = _balances[id][from].amount;
require(uint256(fromBalance) >= amount, "ERC1155: insufficient balance for transfer");

If amount exceeds type(uint128).max, _amount will be truncated to type(uint128).max, and require will still check the original amount, which may cause incorrect state updates.

  1. For the to account:

_balances[id][to].amount = _balances[id][to].amount.add(_amount);

The actual amount received by the to account is _amount (the truncated amount), not the expected amount.

Code Snippet

https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/tokens/Fertilizer/Internalizer.sol#L71-L85
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/tokens/Fertilizer/Fertilizer1155.sol#L77
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/tokens/Fertilizer/Fertilizer1155.sol#L19-L48
https://github.com/Cyfrin/2024-05-beanstalk-the-finale/blob/8c8710df547f7d7c5dd82c5381eb6b34532e4484/protocol/contracts/tokens/Fertilizer/Fertilizer1155.sol#L50-L75

Impact

Internalizer::_transfer() unsafe data type conversion may cause serious problems

Tools Used

Manual Review

Recommendations

Perform checks before conversion

function _transfer(
address from,
address to,
uint256 id,
uint256 amount
) internal virtual override {
+ require(amount <= type(uint128).max, "ERC1155: transfer amount exceeds uint128 max value");
uint128 _amount = uint128(amount);
if (from != address(0)) {
uint128 fromBalance = _balances[id][from].amount;
require(uint256(fromBalance) >= amount, "ERC1155: insufficient balance for transfer");
// Because we know fromBalance >= amount, we know amount < type(uint128).max
_balances[id][from].amount = fromBalance - _amount;
}
_balances[id][to].amount = _balances[id][to].amount.add(_amount);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.