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 11 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
7 changes: 7 additions & 0 deletions .solcover.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,11 @@ module.exports = {
grep: "@skip-on-coverage", // Find everything with this tag
invert: true, // Run the grep's inverse set.
},
configureYulOptimizer: true,
solcOptimizerDetails: {
yul: true,
yulDetails: {
optimizerSteps: ""
},
},
};
4 changes: 2 additions & 2 deletions contracts/Safe.sol
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ contract Safe is
uint256 contractSignatureLen;
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
assembly ("memory-safe") {
contractSignatureLen := mload(add(add(signatures, offset), 0x20))
}
/* solhint-enable no-inline-assembly */
Expand All @@ -273,7 +273,7 @@ contract Safe is
bytes memory contractSignature;
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
assembly ("memory-safe") {
// The signature data for contract signatures is appended to the concatenated signatures and the offset is stored in s
contractSignature := add(add(signatures, offset), 0x20)
}
Expand Down
2 changes: 1 addition & 1 deletion contracts/base/Executor.sol
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ abstract contract Executor {
if (operation == Enum.Operation.DelegateCall) {
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
assembly ("memory-safe") {
success := delegatecall(txGas, to, add(data, 0x20), mload(data), 0, 0)
}
/* solhint-enable no-inline-assembly */
Expand Down
2 changes: 1 addition & 1 deletion contracts/base/FallbackManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ abstract contract FallbackManager is SelfAuthorized {
bytes32 slot = FALLBACK_HANDLER_STORAGE_SLOT;
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
assembly ("memory-safe") {
sstore(slot, handler)
}
/* solhint-enable no-inline-assembly */
Expand Down
2 changes: 1 addition & 1 deletion contracts/base/ModuleManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ abstract contract ModuleManager is SelfAuthorized, Executor, GuardManager {
// Set correct size of returned array
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
assembly ("memory-safe") {
mstore(array, moduleCount)
}
/* solhint-enable no-inline-assembly */
Expand Down
6 changes: 4 additions & 2 deletions contracts/common/SecuredTokenTransfer.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ 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
assembly {
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly ("memory-safe") {
// 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
let success := call(sub(gas(), 10000), token, 0, add(data, 0x20), mload(data), 0, 0x20)
Expand All @@ -34,5 +35,6 @@ abstract contract SecuredTokenTransfer {
transferred := 0
}
}
/* solhint-enable no-inline-assembly */
}
}
2 changes: 1 addition & 1 deletion contracts/common/SignatureDecoder.sol
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ abstract contract SignatureDecoder {
function signatureSplit(bytes memory signatures, uint256 pos) internal pure returns (uint8 v, bytes32 r, bytes32 s) {
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
assembly ("memory-safe") {
let signaturePos := mul(0x41, pos)
r := mload(add(signatures, add(signaturePos, 0x20)))
s := mload(add(signatures, add(signaturePos, 0x40)))
Expand Down
4 changes: 2 additions & 2 deletions contracts/common/StorageAccessible.sol
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ abstract contract StorageAccessible {
for (uint256 index = 0; index < length; index++) {
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
assembly ("memory-safe") {
let word := sload(add(offset, index))
mstore(add(add(result, 0x20), mul(index, 0x20)), word)
}
Expand All @@ -42,7 +42,7 @@ abstract contract StorageAccessible {
function simulateAndRevert(address targetContract, bytes memory calldataPayload) external {
/* solhint-disable no-inline-assembly */
/// @solidity memory-safe-assembly
assembly {
assembly ("memory-safe") {
let success := delegatecall(gas(), targetContract, add(calldataPayload, 0x20), mload(calldataPayload), 0, 0)
// Load free memory location
let ptr := mload(0x40)
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
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
21 changes: 13 additions & 8 deletions contracts/proxies/SafeProxy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -31,21 +31,26 @@ contract SafeProxy {

/// @dev Fallback function forwards all transactions and returns all received return data.
fallback() external payable {
// solhint-disable-next-line no-inline-assembly
/* solhint-disable no-inline-assembly */
remedcu marked this conversation as resolved.
Show resolved Hide resolved
/// @solidity memory-safe-assembly
remedcu marked this conversation as resolved.
Show resolved Hide resolved
assembly {
let _singleton := and(sload(0), 0xffffffffffffffffffffffffffffffffffffffff)
let ptr := mload(0x40)
// 0xa619486e == keccak("masterCopy()"). The value is right padded to 32-bytes with 0s
if eq(calldataload(0), 0xa619486e00000000000000000000000000000000000000000000000000000000) {
mstore(0, _singleton)
return(0, 0x20)
mstore(ptr, _singleton)
remedcu marked this conversation as resolved.
Show resolved Hide resolved
return(ptr, 0x20)
}
calldatacopy(0, 0, calldatasize())
let success := delegatecall(gas(), _singleton, 0, calldatasize(), 0, 0)
returndatacopy(0, 0, returndatasize())
ptr := mload(0x40)
calldatacopy(ptr, 0, calldatasize())
let success := delegatecall(gas(), _singleton, ptr, calldatasize(), 0, 0)
ptr := mload(0x40)
returndatacopy(ptr, 0, returndatasize())
if eq(success, 0) {
revert(0, returndatasize())
revert(ptr, returndatasize())
}
return(0, returndatasize())
return(ptr, returndatasize())
}
/* solhint-enable no-inline-assembly */
}
}
9 changes: 6 additions & 3 deletions contracts/test/DelegateCaller.sol
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,15 @@ 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 */
/// @solidity memory-safe-assembly
assembly {
let ptr := mload(0x40)
remedcu marked this conversation as resolved.
Show resolved Hide resolved
let length := returndatasize()
returndatacopy(0, 0, length)
revert(0, length)
returndatacopy(ptr, 0, length)
revert(ptr, length)
}
/* solhint-enable no-inline-assembly */
}
}
}
2 changes: 2 additions & 0 deletions docs/guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ The data can be stored to this location with
```
bytes32 slot = VARIABLE_SLOT;
// solhint-disable-next-line no-inline-assembly
remedcu marked this conversation as resolved.
Show resolved Hide resolved
/// @solidity memory-safe-assembly
assembly {
sstore(slot, value)
}
Expand All @@ -21,6 +22,7 @@ and read with
```
bytes32 slot = VARIABLE_SLOT;
// solhint-disable-next-line no-inline-assembly
/// @solidity memory-safe-assembly
assembly {
value := sload(slot)
}
Expand Down
13 changes: 10 additions & 3 deletions hardhat.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,10 @@ 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 soliditySettings = SOLIDITY_SETTINGS ? JSON.parse(SOLIDITY_SETTINGS) : undefined;
const primarySolidityVersion = SOLIDITY_VERSION || "0.8.19";
const soliditySettings = SOLIDITY_SETTINGS
? JSON.parse(SOLIDITY_SETTINGS)
: JSON.parse('{"viaIR":true,"optimizer":{"enabled":true, "details": {"yul": true}}}');

const deterministicDeployment = (network: string): DeterministicDeploymentInfo => {
const info = getSingletonFactoryInfo(parseInt(network));
Expand Down Expand Up @@ -70,7 +72,12 @@ 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: "0.7.6" },
{ version: "0.6.12" },
{ version: "0.5.17" },
],
},
networks: {
hardhat: {
Expand Down
53 changes: 13 additions & 40 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion 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 Expand Up @@ -70,7 +71,7 @@
"husky": "^8.0.3",
"prettier": "^3.1.0",
"prettier-plugin-solidity": "^1.2.0",
"solc": "0.7.6",
"solc": "0.8.19",
"solhint": "^4.0.0",
"ts-node": "^10.9.1",
"typescript": "^5.3.2",
Expand Down
4 changes: 2 additions & 2 deletions test/accessors/SimulateTxAccessor.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe("SimulateTxAccessor", () => {
const simulation = accessor.interface.decodeFunctionResult("simulate", acccessibleData);
expect(safe.interface.decodeFunctionResult("getOwners", simulation.returnData)[0]).to.be.deep.eq([user1.address]);
expect(simulation.success).to.be.true;
expect(simulation.estimate).to.be.lte(10000n);
expect(simulation.estimate).to.be.lte(14800n);
remedcu marked this conversation as resolved.
Show resolved Hide resolved
});

it("simulate delegatecall", async () => {
Expand All @@ -83,7 +83,7 @@ describe("SimulateTxAccessor", () => {
userBalance + ethers.parseEther("1"),
);
expect(simulation.success).to.be.true;
expect(simulation.estimate).to.be.lte(15000n);
expect(simulation.estimate).to.be.lte(15100n);
});

it("simulate selfdestruct", async () => {
Expand Down
Loading
Loading