`OutOfBounds` error with openzeppelin ERC20 example

[xermicus]

The code snippet below was reported to generate an OOB memory access during _mint() in the constructor.

Openzeppelin version: 5.2.0

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.28;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Pausable.sol";
import "@openzeppelin/contracts/access/Ownable.sol";

contract MyToken is ERC20, ERC20Burnable, ERC20Pausable, Ownable {
    uint256 public constant MAX_SUPPLY = 10_000_000 * 10**18; // 10 million tokens
    
    constructor(address initialOwner) 
        ERC20("MyToken", "MTK") 
        Ownable(initialOwner)
    {
        _mint(msg.sender, 1_000_000 * 10**decimals()); // Initial supply of 1 million tokens
    }

    function mint(address to, uint256 amount) public onlyOwner {
        require(totalSupply() + amount <= MAX_SUPPLY, "Would exceed max supply");
        _mint(to, amount);
    }

    function pause() public onlyOwner {
        _pause();
    }

    function unpause() public onlyOwner {
        _unpause();
    }

    // Override required by Solidity to handle both ERC20Pausable and ERC20
    function _update(address from, address to, uint256 amount)
        internal
        override(ERC20, ERC20Pausable)
    {
        super._update(from, to, amount);
    }
}

[xermicus]

Spent a moment debugging this. The error we observe happens in set_immutable_data. However this contract does not set any immutable data. set_immutable_data observes a bogus length value of 0x01010101 (which is larger than the max len so it traps with this error).

How this works in the contract: Before returning from the constructor, the contract loads the immutable data size global value. If it is non-zero, it sets the immutable data accordingly. In this case, the immutable data size was nonzero.

The recompiler collects immutable values during the LLVM translation, so we only know the count of immutable variables at the end of the translation but not necessarily while emitting the constructor code - so I implemented it such that it loads the value from an uninitialized global variable and after the translation - when the amount of immutables is known - a module is finally linked in which defines this global variable with the final immutable data size. When debugging the recompiler I can observe that the recompiler indeed thinks the amount of immutable variable is 0 and sets it accordingly.

Inspecting the shared object, I can confirm that the immutable data size global lands in .sbss:

# llvm-readelf -s erc.sol.MyToken.so
   Num:    Value          Size Type    Bind   Vis       Ndx Name
   347: 0000000000000014     4 OBJECT  GLOBAL DEFAULT     7 __immutable_data_size

# llvm-readelf -S erc.sol.MyToken.so
There are 18 section headers, starting at offset 0x17fb8:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
...
  [ 7] .sbss             NOBITS          0000000000000000 010828 000018 00  WA  0   0  8

Up to this point, everything seems correct to me. The variable should be zero initialized and thus lands in some .bss. Yet at runtime, it loads a bogus (not zero initialized value). I can only reproduce it with this exact contract source - the immutable data feature seems to work in general. Could this be PVM linker (relocation) bug?

Note: Disabling the PVM optimizer does not fix this.

The so:

erc.sol.MyToken.so.zip

[athei]

I can only reproduce it with this exact contract source - the immutable data feature seems to work in general. Could this be PVM linker (relocation) bug?

Smells like a linker bug indeed. @koute can we have a release so we can bump pallet_revive to include paritytech/polkavm#265?

[xermicus]

Ah sorry I forgot to mention, I already tried PVM with this patch but the bug is still reproducible (FWIW I'm seeing a r_riscv_got_hi20 relocation at the relevant place).

[athei]

@aman4150 Can you download the object file above and check if the linker is doing the right thing?

[aman4150]

We have internally confirmed that this is a compiler issue and not the linker.

TL;DR - Relocations are correct but the compiler emits 32-bit indirect load instead of 64-bit.