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

Making contracts memory-safe #711

Merged
merged 57 commits into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
22ee559
Hardhat config updated
remedcu Dec 4, 2023
47c9adf
Memory Safe Contracts Written
remedcu Dec 4, 2023
5ee3905
Tests Updated
remedcu Dec 4, 2023
8753bc7
Guidelines Updated
remedcu Dec 4, 2023
d9a6f0c
safe-memory tag added
remedcu Dec 5, 2023
ebf89bb
solhint tags added for inline assembly
remedcu Dec 5, 2023
5cd5e53
SafeProxy assembly updated
remedcu Dec 5, 2023
d9cd9bb
Tests updated for coverage changes
remedcu Dec 5, 2023
4995c3d
solc 0.8.19
remedcu Dec 5, 2023
e287ca7
solcover updated with optimizer config
remedcu Dec 5, 2023
7444b95
fmt command added to package.json
remedcu Dec 5, 2023
c74ef15
solc reverted back to 0.7.6
remedcu Dec 5, 2023
a1a7ce6
Tests updated for coverage gas issue
remedcu Dec 5, 2023
e32f08e
console.logs for checking added
remedcu Dec 5, 2023
3a7bf79
Increased gas for a test with coverage
remedcu Dec 5, 2023
6481742
solc 0.8.19
remedcu Dec 5, 2023
c85784a
applyHarness updated
remedcu Dec 5, 2023
24d5abe
certora applyHarness update
remedcu Dec 5, 2023
5c23638
Reverting back to 0.7.6
remedcu Dec 21, 2023
f715936
Merge pull request #718 from safe-global/optimizer-enabled-with-origi…
remedcu Dec 21, 2023
7dbd1fe
SecuredTokenTransfer made to use free memory instead of scratch space
remedcu Dec 21, 2023
32ce306
viaIR: false in sample env
remedcu Dec 21, 2023
539f2fd
viaIR: false in sample env
remedcu Dec 21, 2023
aa7738a
Merge branch 'optimizer-enabled' of https://github.com/safe-global/sa…
remedcu Dec 21, 2023
534e3f4
Optimization commented and memory-safe edited
remedcu Dec 22, 2023
9166949
Review based safe-memory changes
remedcu Dec 22, 2023
771c969
Remove in assembly of FallbackManager
remedcu Dec 22, 2023
830bfc5
Using data size directly
remedcu Dec 22, 2023
62dcd4a
Using scratch space for in SecuredTokenTransfer
remedcu Dec 22, 2023
149f7e2
removed
remedcu Dec 22, 2023
0586ca8
Test based changes reverted
remedcu Dec 22, 2023
d052266
Comment reinstated
remedcu Dec 22, 2023
e55d181
Comment reinstated at the correct place
remedcu Dec 22, 2023
9c80418
Slight optimization in ModuleManager
remedcu Dec 22, 2023
fbef92b
Comment typo updated
remedcu Jan 5, 2024
625d96d
Version Overrides for old OZ files
remedcu Jan 5, 2024
b723796
Updating old compiler versions to default one
remedcu Jan 5, 2024
edc2910
Updated harness patch
remedcu Jan 8, 2024
9740605
SafeProxy using scratch space initially
remedcu Jan 8, 2024
0bdca8a
Updated harness patch
remedcu Jan 8, 2024
1cce1e9
Certora Harness Restored
remedcu Jan 8, 2024
5d2df08
EOF for .env.sample
remedcu Jan 8, 2024
5919110
Sigleton -> Singleton typo fixed
remedcu Jan 8, 2024
76a48cc
.env.sample updation
remedcu Jan 8, 2024
50e3d0c
harness timestamp restored
remedcu Jan 8, 2024
81a0fc4
Test Token pragma raised
remedcu Jan 8, 2024
082c7af
solcover and .env updated
remedcu Jan 9, 2024
bc16a01
Using scratch space for call result boolean
remedcu Jan 9, 2024
d9069e2
Guidelines Updated
remedcu Jan 9, 2024
36280d3
solhint inline assembly comment
remedcu Jan 9, 2024
d3f6010
SafeProxy memory-safety reverted
remedcu Jan 10, 2024
6b25caf
Multisend* memory-safety revert
remedcu Jan 10, 2024
c53fdc7
Using returndatasize directly
remedcu Jan 10, 2024
cd87eb0
DelegateCaller memory-safe revert and using returndatasize
remedcu Jan 10, 2024
7c51578
SafeProxy all changes reverted
remedcu Jan 10, 2024
0d24013
Using returndatasize through memory (revert)
remedcu Jan 10, 2024
6230350
contract.ts memory-safe tagging reverted
remedcu Jan 10, 2024
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
6 changes: 5 additions & 1 deletion .env.sample
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,8 @@ ERC4337_TEST_NODE_URL=
ERC4337_TEST_SINGLETON_ADDRESS=
ERC4337_TEST_SAFE_FACTORY_ADDRESS=
# (Optional) Tells the test runner which Safe Singleton Contract to use for testing: Safe or SafeL2. Defaults to Safe.
SAFE_CONTRACT_UNDER_TEST="Safe"
SAFE_CONTRACT_UNDER_TEST="Safe"
# Used for compiling with different solidity testing
SOLIDITY_VERSION= # Example: '0.8.19'
# For running coverage tests, `details` section of solidity settings are required, else could be removed.
SOLIDITY_SETTINGS= # Example: '{"viaIR":true,"optimizer":{"enabled":true, "details": {"yul": true, "yulDetails": { "optimizerSteps": ""}}}}'
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ coverage/
coverage.json
yarn-error.log
typechain-types
.vscode

# Certora Formal Verification related files
.certora_internal
Expand Down
2 changes: 1 addition & 1 deletion certora/specs/Safe.spec
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ hook Sstore SafeHarness.(slot 24440054405305269366569402256811496959409073762505
ghostSingletonAddress = newSingletonAddress;
}

invariant sigletonAddressNeverChanges()
invariant singletonAddressNeverChanges()
ghostSingletonAddress == 0
filtered { f -> reachableOnly(f) && f.selector != sig:getStorageAt(uint256,uint256).selector }

Expand Down
21 changes: 7 additions & 14 deletions contracts/base/FallbackManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -69,33 +69,26 @@ abstract contract FallbackManager is SelfAuthorized {
// memory is used, therefore we need to guarantee memory safety (keeping the free memory point 0x40 slot intact,
// not going beyond the scratch space, etc)
// Solidity docs: https://docs.soliditylang.org/en/latest/assembly.html#memory-safety
function allocate(length) -> pos {
pos := mload(0x40)
mstore(0x40, add(pos, length))
}

let handler := sload(slot)
if iszero(handler) {
return(0, 0)
}

let calldataPtr := allocate(calldatasize())
calldatacopy(calldataPtr, 0, calldatasize())
let ptr := mload(0x40)
remedcu marked this conversation as resolved.
Show resolved Hide resolved
calldatacopy(ptr, 0, calldatasize())

// The msg.sender address is shifted to the left by 12 bytes to remove the padding
// Then the address without padding is stored right after the calldata
let senderPtr := allocate(20)
mstore(senderPtr, shl(96, caller()))
mstore(add(ptr, calldatasize()), shl(96, caller()))

// Add 20 bytes for the address appended add the end
let success := call(gas(), handler, 0, calldataPtr, add(calldatasize(), 20), 0, 0)
let success := call(gas(), handler, 0, ptr, add(calldatasize(), 20), 0, 0)

let returnDataPtr := allocate(returndatasize())
returndatacopy(returnDataPtr, 0, returndatasize())
returndatacopy(ptr, 0, returndatasize())
if iszero(success) {
revert(returnDataPtr, returndatasize())
revert(ptr, returndatasize())
}
return(returnDataPtr, returndatasize())
return(ptr, returndatasize())
}
/* solhint-enable no-inline-assembly */
}
Expand Down
10 changes: 4 additions & 6 deletions contracts/base/ModuleManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,14 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager {
/// @solidity memory-safe-assembly
assembly {
// Load free memory location
let ptr := mload(0x40)
returnData := mload(0x40)
Copy link
Member

Choose a reason for hiding this comment

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

that's a good catch sir

Copy link
Collaborator

Choose a reason for hiding this comment

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

Eagle-eyes!

// We allocate memory for the return data by setting the free memory location to
// current free memory location + data size + 32 bytes for data size value
mstore(0x40, add(ptr, add(returndatasize(), 0x20)))
mstore(0x40, add(returnData, add(returndatasize(), 0x20)))
// Store the size
mstore(ptr, returndatasize())
mstore(returnData, returndatasize())
// Store the data
returndatacopy(add(ptr, 0x20), 0, returndatasize())
// Point the return data to the correct memory location
returnData := ptr
returndatacopy(add(returnData, 0x20), 0, returndatasize())
}
/* solhint-enable no-inline-assembly */
}
Expand Down
4 changes: 3 additions & 1 deletion contracts/common/SecuredTokenTransfer.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ abstract contract SecuredTokenTransfer {
function transferToken(address token, address receiver, uint256 amount) internal returns (bool transferred) {
// 0xa9059cbb - keccack("transfer(address,uint256)")
bytes memory data = abi.encodeWithSelector(0xa9059cbb, receiver, amount);
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
// We write the return value to scratch space.
remedcu marked this conversation as resolved.
Show resolved Hide resolved
// See https://docs.soliditylang.org/en/v0.7.6/internals/layout_in_memory.html#layout-in-memory
Expand All @@ -34,5 +35,6 @@ abstract contract SecuredTokenTransfer {
transferred := 0
}
}
/* solhint-enable no-inline-assembly */
}
}
9 changes: 5 additions & 4 deletions contracts/handler/CompatibilityFallbackHandler.sol
Original file line number Diff line number Diff line change
Expand Up @@ -97,20 +97,21 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
let internalCalldata := mload(0x40)
let ptr := mload(0x40)
/**
* Store `simulateAndRevert.selector`.
* String representation is used to force right padding
*/
mstore(internalCalldata, "\xb4\xfa\xba\x09")
mstore(ptr, "\xb4\xfa\xba\x09")

/**
* Abuse the fact that both this and the internal methods have the
* same signature, and differ only in symbol name (and therefore,
* selector) and copy calldata directly. This saves us approximately
* 250 bytes of code and 300 gas at runtime over the
* `abi.encodeWithSelector` builtin.
*/
calldatacopy(add(internalCalldata, 0x04), 0x04, sub(calldatasize(), 0x04))
calldatacopy(add(ptr, 0x04), 0x04, sub(calldatasize(), 0x04))

/**
* `pop` is required here by the compiler, as top level expressions
Expand All @@ -124,7 +125,7 @@ contract CompatibilityFallbackHandler is TokenCallbackHandler, ISignatureValidat
// address() has been changed to caller() to use the implementation of the Safe
caller(),
0,
internalCalldata,
ptr,
calldatasize(),
/**
* The `simulateAndRevert` call always reverts, and
Expand Down
1 change: 1 addition & 0 deletions contracts/interfaces/ViewStorageAccessible.sol
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// SPDX-License-Identifier: LGPL-3.0-only
remedcu marked this conversation as resolved.
Show resolved Hide resolved
pragma solidity >=0.5.0 <0.9.0;

/// @title ViewStorageAccessible - Interface on top of StorageAccessible base class to allow simulations from view functions.
Expand Down
1 change: 0 additions & 1 deletion contracts/libraries/MultiSend.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ contract MultiSend {
function multiSend(bytes memory transactions) public payable {
require(address(this) != MULTISEND_SINGLETON, "MultiSend should only be called via delegatecall");
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
let length := mload(transactions)
let i := 0x20
Expand Down
1 change: 0 additions & 1 deletion contracts/libraries/MultiSendCallOnly.sol
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ contract MultiSendCallOnly {
*/
function multiSend(bytes memory transactions) public payable {
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
let length := mload(transactions)
let i := 0x20
Expand Down
4 changes: 3 additions & 1 deletion contracts/libraries/Safe130To141Migration.sol
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,12 @@ contract Safe130To141Migration is SafeStorage {
*/
function isContract(address account) internal view returns (bool) {
uint256 size;
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
size := extcodesize(account)
}
/* solhint-enable no-inline-assembly */

// If the code size is greater than 0, it is a contract; otherwise, it is an EOA.
return size > 0;
Expand Down
8 changes: 6 additions & 2 deletions contracts/libraries/Safe150Migration.sol
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,12 @@ contract Safe150Migration is SafeStorage {
*/
function getGuard() internal view returns (address guard) {
bytes32 slot = GUARD_STORAGE_SLOT;
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
guard := sload(slot)
}
/* solhint-enable no-inline-assembly */
}

/**
Expand All @@ -176,10 +178,12 @@ contract Safe150Migration is SafeStorage {
*/
function isContract(address account) internal view returns (bool) {
uint256 size;
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
size := extcodesize(account)
}
/* solhint-enable no-inline-assembly */

// If the code size is greater than 0, it is a contract; otherwise, it is an EOA.
return size > 0;
Expand Down
6 changes: 4 additions & 2 deletions contracts/libraries/SafeToL2Migration.sol
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ contract SafeToL2Migration is SafeStorage {
0,
0,
address(0),
address(0),
payable(address(0)),
remedcu marked this conversation as resolved.
Show resolved Hide resolved
"", // We cannot detect signatures
additionalInfo
);
Expand Down Expand Up @@ -166,10 +166,12 @@ contract SafeToL2Migration is SafeStorage {
*/
function isContract(address account) internal view returns (bool) {
uint256 size;
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
size := extcodesize(account)
}
/* solhint-enable no-inline-assembly */

// If the code size is greater than 0, it is a contract; otherwise, it is an EOA.
return size > 0;
Expand Down
3 changes: 2 additions & 1 deletion contracts/test/DelegateCaller.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@ contract DelegateCaller {
function makeDelegatecall(address _called, bytes memory _calldata) external returns (bool success, bytes memory returnData) {
(success, returnData) = _called.delegatecall(_calldata);
if (!success) {
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
assembly {
let length := returndatasize()
returndatacopy(0, 0, length)
revert(0, length)
}
/* solhint-enable no-inline-assembly */
}
}
}
2 changes: 1 addition & 1 deletion contracts/test/Token.sol
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity >=0.6.0 <0.7.0;
pragma solidity >=0.6.0 <0.9.0;

interface Token {
function transfer(address _to, uint256 value) external returns (bool);
Expand Down
8 changes: 6 additions & 2 deletions docs/guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,24 @@ The data can be stored to this location with

```
bytes32 slot = VARIABLE_SLOT;
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
sstore(slot, value)
}
/* solhint-enable no-inline-assembly */
```

and read with

```
bytes32 slot = VARIABLE_SLOT;
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
value := sload(slot)
}
/* solhint-enable no-inline-assembly */
```

Note: Make sure to use a unique identifier else unexpected behaviour will occur
Expand Down
5 changes: 3 additions & 2 deletions hardhat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ import "./src/tasks/show_codesize";
import { BigNumber } from "@ethersproject/bignumber";
import { DeterministicDeploymentInfo } from "hardhat-deploy/dist/types";

const primarySolidityVersion = SOLIDITY_VERSION || "0.7.6";
const defaultSolidityVersion = "0.7.6";
const primarySolidityVersion = SOLIDITY_VERSION || defaultSolidityVersion;
const soliditySettings = SOLIDITY_SETTINGS ? JSON.parse(SOLIDITY_SETTINGS) : undefined;

const deterministicDeployment = (network: string): DeterministicDeploymentInfo => {
Expand Down Expand Up @@ -70,7 +71,7 @@ const userConfig: HardhatUserConfig = {
target: "ethers-v6",
},
solidity: {
compilers: [{ version: primarySolidityVersion, settings: soliditySettings }, { version: "0.6.12" }, { version: "0.5.17" }],
compilers: [{ version: primarySolidityVersion, settings: soliditySettings }, { version: defaultSolidityVersion }],
remedcu marked this conversation as resolved.
Show resolved Hide resolved
},
networks: {
hardhat: {
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"lint:sol:prettier": "prettier 'contracts/**/*.sol' --list-different",
"lint:ts": "eslint 'src/**/*.ts' 'test/**/*.ts' --fix",
"lint:ts:prettier": "prettier 'src/**/*.ts' 'test/**/*.ts' --list-different",
"fmt": "npm run fmt:sol && npm run fmt:ts",
"fmt:sol": "prettier 'contracts/**/*.sol' -w",
"fmt:ts": "prettier 'src/**/*.ts' 'test/**/*.ts' -w",
"prepack": "npm run build",
Expand Down
4 changes: 3 additions & 1 deletion test/core/Safe.Execution.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@ describe("Safe", () => {
contract StorageSetter {
function setStorage(bytes3 data) public {
bytes32 slot = 0x4242424242424242424242424242424242424242424242424242424242424242;
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
sstore(slot, data)
}
/* solhint-enable no-inline-assembly */
}
}`;
const storageSetter = await deployContract(user1, setterSource);
Expand Down
4 changes: 3 additions & 1 deletion test/core/Safe.Setup.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,10 +216,12 @@ describe("Safe", () => {
contract Initializer {
function init(bytes4 data) public {
bytes32 slot = 0x4242424242424242424242424242424242424242424242424242424242424242;
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
sstore(slot, data)
}
/* solhint-enable no-inline-assembly */
}
}`;
const testIntializer = await deployContract(user1, source);
Expand Down
4 changes: 3 additions & 1 deletion test/libraries/MultiSend.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ describe("MultiSend", () => {
contract StorageSetter {
function setStorage(bytes3 data) public {
bytes32 slot = 0x4242424242424242424242424242424242424242424242424242424242424242;
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
sstore(slot, data)
}
/* solhint-enable no-inline-assembly */
}
}`;
const signers = await ethers.getSigners();
Expand Down
4 changes: 3 additions & 1 deletion test/libraries/MultiSendCallOnly.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ describe("MultiSendCallOnly", () => {
contract StorageSetter {
function setStorage(bytes3 data) public {
bytes32 slot = 0x4242424242424242424242424242424242424242424242424242424242424242;
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
sstore(slot, data)
}
/* solhint-enable no-inline-assembly */
}
}`;
const signers = await ethers.getSigners();
Expand Down
Loading