Critical bug: allowance storage slot can be override with a transfer operation

[yaronvel]

transfer and transferFrom do not check that to does not exceed 20 bytes. As a result, malicious transfer operation can override allowance slot.

The following test pass:

  function testRug() public {
    address bob = address(0x123);
    address alice = address(0x456);

    uint allowanceSlot = uint(keccak256(abi.encode(bob, alice)));
    assertEq(token.allowance(bob, alice), 0);   
    address(token).call(      abi.encodePacked(bytes4(keccak256(bytes("transfer(address,uint256)"))), allowanceSlot, uint(1))      );
    assertEq(token.allowance(bob, alice), 1);
  }

[outdoteth]

Awesome find. I think the cheapest fix is this:

PUSH32 (1 << 160) - 1 PUSH 0x04 CALLDATALOAD AND

wydt?

example

I think you can CALLDATACOPY the addy too but that puts it in memory which is more expensive.

[yaronvel]

Do you assume that the result of the hash will never have all 12 leading bytes zero? It is somewhat reasonable, but very non trivial assumption.

If you want to avoid this assumption, you can store balances at even storage slots, and allowances at odds slots (just multiply by 2 and add 1 for odd slots).

[outdoteth]

PUSH1 0x04 CALLDATALOAD PUSH1 0x02 MUL PUSH1 0x01 ADD

Ok that is obviously more robust. I don’t think my assumption was reasonable actually. Say Bob has not approved Alice. Alice can mine for an address that would result in an allowance storage slot which has 12 leading zeroes. Not sure if that’s in the realm of possibility but I think it might be.

[yaronvel]

Maybe this is more efficient (playing with bit 255 instead of bit 0)

For balance storage slot:

...
PUSH32 0x7FFFF....F // (all bits are set beside bit 255)
AND

For allowance storage slot:

...
PUSH32 0x8000...0 // (bit 255 is set. the rest are 0)
OR

From what I get PUSH32 cost the same as PUSH1 (in runtime, deployment costs are higher). AND and OR are cheaper than MUL. And it saves two ops for allowance slot calculation.

[outdoteth]

Nice. What’s the reason for marking the 255 bit instead of the 0 bit? Is it so that you don’t lose 1 bit on the address?

[yaronvel]

Yes. But might be ok to mask bit 0 if it helps somehow. To save a bit of deployment gas can mask bit 160 instead of 255.