Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: superc20 tob properties #27

Merged
merged 7 commits into from
Aug 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,6 @@
[submodule "packages/contracts-bedrock/lib/halmos-cheatcodes"]
path = packages/contracts-bedrock/lib/halmos-cheatcodes
url = https://github.com/a16z/halmos-cheatcodes
[submodule "packages/contracts-bedrock/lib/properties"]
path = packages/contracts-bedrock/lib/properties
url = https://github.com/crytic/properties
1 change: 1 addition & 0 deletions packages/contracts-bedrock/foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ remappings = [
'@rari-capital/solmate/=lib/solmate',
'@lib-keccak/=lib/lib-keccak/contracts/lib',
'@solady/=lib/solady/src',
'@crytic/properties/=lib/properties/',
'forge-std/=lib/forge-std/src',
'ds-test/=lib/forge-std/lib/ds-test/src',
'safe-contracts/=lib/safe-contracts/contracts',
Expand Down
1 change: 1 addition & 0 deletions packages/contracts-bedrock/lib/properties
Submodule properties added at bb1b78
6 changes: 5 additions & 1 deletion packages/contracts-bedrock/medusa.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,11 @@
]
},
"targetFunctionSignatures": [],
"excludeFunctionSignatures": []
"excludeFunctionSignatures": [
"ProtocolProperties.test_ERC20external_zeroAddressBalance()",
"ProtocolProperties.test_ERC20external_transferToZeroAddress()",
"ProtocolProperties.test_ERC20external_transferFromToZeroAddress(uint256)"
Comment on lines +60 to +61
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't an assumption somewhere else in the codebase, right (like in a mint or burn or smth)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it's not checked anywhere else and I didn't find it in the docs either. Perhaps it's worth checking with @0xParticle :

  • Is it okay for transfers to the zero address to succeed?
  • Is it okay for transferFroms to the zero address to succeed?
  • and, is it okay for the above to update the balance of address(0), so it could be non-zero?

the above implies there will not be a bijection between OptimismSuperchainERC20 Transfer({from: somebody, to: address(0), amount: something}) events and sendERC20 calls

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually an historical discussion. The ERC20 standard does not mention anything about these scenarios, so implementation can block or not transfers to the zero address. Same for tracking the balance of the zero address.

  • OZ, for instance, does check and revert on transfers to the zero address. This is an opinionated design, but its fine as it can prevent potential bugs.
  • The implementation you are testing uses solady ERC20 which doesn't check for zero address on transfers for gas optimization purposes.
    Also, keep in mind that transfers to the zero address have a use case, which is to effectively burn tokens.

So, answering the 3 bullet points, the three scenarios are ok as its ERC20 compliant.

Regarding the symmetry between transfers and bridging, I think its fine.

  • As I said, the primary use case for transfering tokens to the zero address is "burning", and that action is already available using the transfer and transferFrom methods in the solady implementation. I don't see why someone would do it via a bridge, unless they want to waste gas.
  • That said, blocking the bridging to the zero address is an opinionated design choice, but it does feel like touching the bridge should be reserved to the intended use of it (clients will be grateful).

]
},
"chainConfig": {
"codeSizeCheckDisabled": true,
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts-bedrock/test/properties/PROPERTIES.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ It’s worth noting that these properties will not hold for a live system

# Expected external interactions

- regular ERC20 operations between any accounts on the same chain, provided by [crytic ERC20 properties](https://github.com/crytic/properties?tab=readme-ov-file#erc20-tests)
- [x] regular ERC20 operations between any accounts on the same chain, provided by [crytic ERC20 properties](https://github.com/crytic/properties?tab=readme-ov-file#erc20-tests)

# Invariant-breaking candidates (brain dump)

Expand Down
10 changes: 7 additions & 3 deletions packages/contracts-bedrock/test/properties/helpers/Actors.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,20 @@ contract Actors is StdUtils {
/// usually called with msg.sender as a parameter, to track the actors
/// already provided by the fuzzer
modifier withActor(address who) {
addActor(who);
_currentActor = who;
_;
}

function addActor(address who) internal {
if (!_isActor[who]) {
_isActor[who] = true;
_actors.push(who);
}
_currentActor = who;
_;
}

/// @notice get the currently configured actor, should equal msg.sender
function currentActor() internal view returns(address){
function currentActor() internal view returns (address) {
return _currentActor;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// SPDX-License-Identifier: AGPL-3
pragma solidity ^0.8.25;

import { OptimismSuperchainERC20 } from "src/L2/OptimismSuperchainERC20.sol";

contract OptimismSuperchainERC20ForToBProperties is OptimismSuperchainERC20 {
bool public constant isMintableOrBurnable = true;
uint256 public initialSupply = 0;
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,30 @@ pragma solidity ^0.8.25;

import { TestBase } from "forge-std/Base.sol";

import { ITokenMock } from "@crytic/properties/contracts/ERC20/external/util/ITokenMock.sol";
import { EnumerableMap } from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol";
import { CryticERC20ExternalBasicProperties } from
"@crytic/properties/contracts/ERC20/external/properties/ERC20ExternalBasicProperties.sol";
import { ProtocolHandler } from "./handlers/Protocol.handler.t.sol";
import { OptimismSuperchainERC20 } from "src/L2/OptimismSuperchainERC20.sol";

contract ProtocolProperties is ProtocolHandler {
contract ProtocolProperties is ProtocolHandler, CryticERC20ExternalBasicProperties {
simon-something marked this conversation as resolved.
Show resolved Hide resolved
using EnumerableMap for EnumerableMap.Bytes32ToUintMap;

/// @dev `token` is the token under test for the ToB properties. This is coupled
/// to the ProtocolHandler constructor initializing at least one supertoken
constructor() {
token = ITokenMock(allSuperTokens[0]);
}

/// @dev not that much of a handler, since this only changes which
/// supertoken the ToB assertions are performed against. Thankfully, they are
/// implemented in a way that don't require tracking ghost variables or can
/// break properties defined by us
function handler_ToBTestOtherToken(uint256 index) external {
token = ITokenMock(allSuperTokens[bound(index, 0, allSuperTokens.length - 1)]);
}

// TODO: will need rework after
// - non-atomic bridge
// - `convert`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { StdUtils } from "forge-std/StdUtils.sol";
import { ERC1967Proxy } from "@openzeppelin/contracts-v5/proxy/ERC1967/ERC1967Proxy.sol";
import { EnumerableMap } from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol";
import { OptimismSuperchainERC20 } from "src/L2/OptimismSuperchainERC20.sol";
import { OptimismSuperchainERC20ForToBProperties } from "../../helpers/OptimismSuperchainERC20ForToBProperties.t.sol";
import { Predeploys } from "src/libraries/Predeploys.sol";
import { MockL2ToL2CrossDomainMessenger } from "../../helpers/MockL2ToL2CrossDomainMessenger.t.sol";
import { Actors } from "../../helpers/Actors.t.sol";
Expand Down Expand Up @@ -37,18 +38,20 @@ contract ProtocolHandler is TestBase, StdUtils, Actors {
address[] internal remoteTokens;
address[] internal allSuperTokens;

//@notice 'real' deploy salt => total supply sum across chains
/// @notice 'real' deploy salt => total supply sum across chains
EnumerableMap.Bytes32ToUintMap internal ghost_totalSupplyAcrossChains;

constructor() {
vm.etch(address(MESSENGER), address(new MockL2ToL2CrossDomainMessenger()).code);
superchainERC20Impl = new OptimismSuperchainERC20();
superchainERC20Impl = new OptimismSuperchainERC20ForToBProperties();
for (uint256 remoteTokenIndex = 0; remoteTokenIndex < INITIAL_TOKENS; remoteTokenIndex++) {
_deployRemoteToken();
for (uint256 supertokenChainId = 0; supertokenChainId < INITIAL_SUPERTOKENS; supertokenChainId++) {
_deploySupertoken(remoteTokens[remoteTokenIndex], WORDS[0], WORDS[0], DECIMALS[0], supertokenChainId);
}
}
// integrate with all ToB properties using address(this) as the sender
addActor(address(this));
}

/// @notice the deploy params are _indexes_ to pick from a pre-defined array of options and limit
Expand Down Expand Up @@ -81,6 +84,52 @@ contract ProtocolHandler is TestBase, StdUtils, Actors {
ghost_totalSupplyAcrossChains.set(MESSENGER.superTokenInitDeploySalts(addr), currentValue + amount);
}

/// @notice The ToB properties don't preclude the need for this since they
/// always use address(this) as the caller, which won't get any balance
/// until it's transferred to it somehow
function handler_SupERC20Transfer(
uint256 tokenIndex,
uint256 toIndex,
uint256 amount
)
external
withActor(msg.sender)
{
vm.prank(currentActor());
OptimismSuperchainERC20(allSuperTokens[bound(tokenIndex, 0, allSuperTokens.length)]).transfer(
getActorByRawIndex(toIndex), amount
);
}

function handler_SupERC20TransferFrom(
uint256 tokenIndex,
uint256 fromIndex,
uint256 toIndex,
uint256 amount
)
external
withActor(msg.sender)
{
vm.prank(currentActor());
OptimismSuperchainERC20(allSuperTokens[bound(tokenIndex, 0, allSuperTokens.length)]).transferFrom(
getActorByRawIndex(fromIndex), getActorByRawIndex(toIndex), amount
);
}

function handler_SupERC20Approve(
uint256 tokenIndex,
uint256 spenderIndex,
uint256 amount
)
external
withActor(msg.sender)
{
vm.prank(currentActor());
OptimismSuperchainERC20(allSuperTokens[bound(tokenIndex, 0, allSuperTokens.length)]).approve(
getActorByRawIndex(spenderIndex), amount
);
}

/// @notice deploy a remote token, that supertokens will be a representation of. They are never called, so there
/// is no need to actually deploy a contract for them
function _deployRemoteToken() internal {
Expand Down