Brechtpd / base64

MIT License
150 stars 33 forks source link

Broken for L2 Arbitrum #3

Open kalepail opened 2 years ago

kalepail commented 2 years ago

I'm assuming maybe assembly isn't supported but would you have any idea why this library doesn't work when deployed on Arbitrum?

kalepail commented 2 years ago

After extensive testing I can confirm this library broke at this commit https://github.com/Brechtpd/base64/commit/0e3627ccdb03ac8c7b7db52600d12d6ce4856e1c for L2 Arbitrum and I assume other L2s as well.

You can see this working via this code here from commit https://github.com/Brechtpd/base64/commit/e78d9fd951e7b0977ddca77d92dc85183770daf4 https://testnet.arbiscan.io/address/0x5a780eb0ae8006BDE6203BFB1a939fd0266f19B2#readContract

It's broken here with this newer base64.sol code however from commit https://github.com/Brechtpd/base64/commit/4d85607b18d981acff392d2e99ba654305552a97 https://testnet.arbiscan.io/address/0xb1452F0488E46e91EeEf61219fE257f91D5eF516#readContract

kalepail commented 2 years ago

Reverting mstore8 back to mstore resolves this issue.

tempe-techie commented 2 years ago

and I assume other L2s as well.

Just FYI: it's not the issue on Optimism. I assume it's the same for Optimism forks.

kalepail commented 2 years ago

There's also something here with running the mload through shl(248, mload(...)). Just swapping mstore8 for mstore produces invalid base64 strings. Adding back the shl wrapper gets everything working again.

Brechtpd commented 2 years ago

Hey, thanks for looking into this. Seems like Arbitrum may have some problems with mstore8, otherwise not much changed. No idea why this would be.

There's also something here with running the mload through shl(248, mload(...)). Just swapping mstore8 for mstore produces invalid base64 strings. Adding back the shl wrapper gets everything working again.

mstore8 only stores the least significant byte, so for mstore to do the same all bytes need to be shifted to the left by 31 bytes so mstore stores the same byte as mstore8 at the same position (+ 31 useless zero bytes to the right of it). mstore8 a bit more efficient so I switched to just doing that.

kalepail commented 2 years ago

I have an outstanding ticket with the Arbitrum team. We'll see what they say. https://bugs.immunefi.com/dashboard/submission/5898

Also and not necessarily related but what's the difference between your library and OpenZeppelin's? https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Base64.sol

They both have the same mstore8 issue for L2 and the same fix (using mstore and shl(248, mload(...)) works on both. Any idea if there's a significant gas difference between yours and theirs?

Thanks for the prompt reply on a Friday 🤯

Brechtpd commented 2 years ago

Haven't compared them, but they are pretty much the same so any difference in gas will be extremely small. The OpenZeppelin implementation is based on my code, they just saw that the library was used frequently and wanted to include it directly in their collection of contracts.