0xsequence / erc-1155

Ethereum Semi Fungible Standard (ERC-1155)
https://sequence.build
Other
320 stars 119 forks source link

M1 - Possible overflow during `_viewUpdateBinValue` #25

Closed PhABC closed 4 years ago

PhABC commented 4 years ago

image

by @Agusx1211

PhABC commented 4 years ago

Good find!

Should be fixed with the following :

function _viewUpdateBinValue(uint256 _binValues, uint256 _index, uint256 _amount, Operations _operation)
  internal pure returns (uint256 newBinValues)
{
  uint256 shift = IDS_BITS_SIZE * _index;
  uint256 mask = (uint256(1) << IDS_BITS_SIZE) - 1;

  if (_operation == Operations.Add) {
    newBinValues = _binValues + (_amount << shift);
    require(newBinValues >= _binValues, "ERC1155PackedBalance#_viewUpdateBinValue: OVERFLOW");
    require(
      ((_binValues >> shift) & mask) + _amount < 2**IDS_BITS_SIZE, // Checks that no other id changed
      "ERC1155PackedBalance#_viewUpdateBinValue: OVERFLOW"
    );

  } else if (_operation == Operations.Sub) {
    newBinValues = _binValues - (_amount << shift);
    require(newBinValues <= _binValues, "ERC1155PackedBalance#_viewUpdateBinValue: UNDERFLOW");
    require(
      ((_binValues >> shift) & mask) >= _amount, // Checks that no other id changed
      "ERC1155PackedBalance#_viewUpdateBinValue: UNDERFLOW"
    );

  } else {
    revert("ERC1155PackedBalance#_viewUpdateBinValue: INVALID_BIN_WRITE_OPERATION"); // Bad operation
  }

  return newBinValues;
}

Refactored this a tiny bit so it consumes less gas too.

Agusx1211 commented 4 years ago

That should fix it