🐛 Mask address args in STL #347

transmissions11 · 2022-12-12T21:32:10Z

Description

Should close the following issues by properly masking address arguments in STL:

#346
#344
#299

Checklist

Ensure you completed all of the steps below before submitting your pull request:

Ran forge snapshot?
Ran npm run lint?
Ran forge test?

Pull requests with an incomplete checklist will be thrown out.

sambacha · 2022-12-12T21:35:01Z

what additional tests would you want? (if any)

transmissions11 · 2022-12-12T21:55:23Z

#299

basically these but + fuzz test versions ideally

sandybradley · 2022-12-13T10:32:21Z

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import "solmate/utils/SafeTransferLib.sol";
import "solmate/tokens/ERC20.sol";

contract ContractTest is Test {

    Token token = new Token();
    Router router = new Router();

    function setUp() public {
        
    }
    function testSafeTransferOk(uint256 amount) public {
        token.approve(address(router), amount);
        router.safeTransferSucceeds(address(token), amount); // Ok.
    }
    function testSafeTransferNok(uint256 amount) public {
        token.approve(address(router), amount);
        router.safeTransferFails(address(token), amount); // This fails.
    }
}

contract Router {
    function safeTransferSucceeds(address token, uint256 amount) public {
        address to = address(uint160(uint256(0x000000000000000000000000ffffffffffffffffffffffffffffffffffffffff))); // type(uint160).max
        SafeTransferLib.safeTransferFrom(ERC20(token), msg.sender, to, amount);
    }
    function safeTransferFails(address token, uint256 amount) public {
        address to = address(uint160(uint256(0x0000000000000000000000010000000000000000000000000000000000000000))); // type(uint160).max + 1
        SafeTransferLib.safeTransferFrom(ERC20(token), msg.sender, to, amount);
    }
}

contract Token is ERC20("", "", 18) {
    constructor() {
        _mint(msg.sender, type(uint256).max);
    }
}

Running 2 tests for test/SafeTransferLib.t.sol:ContractTest
[PASS] testSafeTransferNok(uint256) (runs: 256, μ: 47911, ~: 48612)
Traces:
  [52879] ContractTest::testSafeTransferNok(2)
    ├─ [24523] Token::approve(Router: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 2)
    │   ├─ emit Approval(owner: ContractTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], spender: Router: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], amount: 2)
    │   └─ ← true
    ├─ [24851] Router::safeTransferFails(Token: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 2)
    │   ├─ [24343] Token::transferFrom(ContractTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x0000000000000000000000000000000000000000, 2)
    │   │   ├─ emit Transfer(from: ContractTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: 0x0000000000000000000000000000000000000000, amount: 2)
    │   │   └─ ← true
    │   └─ ← ()
    └─ ← ()

[PASS] testSafeTransferOk(uint256) (runs: 256, μ: 47966, ~: 48566)
Traces:
  [52831] ContractTest::testSafeTransferOk(1889567281)
    ├─ [24523] Token::approve(Router: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], 1889567281)
    │   ├─ emit Approval(owner: ContractTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], spender: Router: [0x2e234DAe75C793f67A35089C9d99245E1C58470b], amount: 1889567281)
    │   └─ ← true
    ├─ [24847] Router::safeTransferSucceeds(Token: [0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f], 1889567281)
    │   ├─ [24343] Token::transferFrom(ContractTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF, 1889567281)
    │   │   ├─ emit Transfer(from: ContractTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: 0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF, amount: 1889567281)
    │   │   └─ ← true
    │   └─ ← ()
    └─ ← ()

Test result: ok. 2 passed; 0 failed; finished in 3.59s

sandybradley · 2022-12-13T20:06:51Z

Or something more complete:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import "solmate/utils/SafeTransferLib.sol";
import "solmate/tokens/ERC20.sol";

contract SafeTransferLibTest is Test {
    using SafeTransferLib for ERC20;

    ERC20 token;
    address user = 0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926;

    function setUp() public {
        vm.prank(user);
        token = new Token();
    }

    function testSafeTransferFrom(address to, uint256 amount) public {
        vm.prank(user);
        ERC20(token).safeApprove(address(this), amount);
        ERC20(token).safeTransferFrom(user, to, amount);
    }

    function testSafeTransfer(address to, uint256 amount) public {
        vm.prank(user);
        ERC20(token).safeTransfer(to, amount);
    }

    function testSafeTransferETH(uint256 amount) public {
        vm.assume(amount < type(uint256).max / 2);
        vm.deal(user, amount);
        vm.prank(user);
        SafeTransferLib.safeTransferETH(address(this), amount);
    }

    receive() external payable {}
    fallback() external payable {}
}

contract Token is ERC20("", "", 18) {
    constructor() {
        _mint(msg.sender, type(uint256).max);
    }
}

Running 3 tests for test/SafeTransferLib.t.sol:SafeTransferLibTest
[PASS] testSafeTransfer(address,uint256) (runs: 256, μ: 39047, ~: 39837)
Traces:
  [39837] SafeTransferLibTest::testSafeTransfer(0xf497BA995A0d145956eBb6358782F60D83556F37, 2)
    ├─ [0] VM::prank(0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926)
    │   └─ ← ()
    ├─ [29674] Token::transfer(0xf497BA995A0d145956eBb6358782F60D83556F37, 2)
    │   ├─ emit Transfer(from: 0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926, to: 0xf497BA995A0d145956eBb6358782F60D83556F37, amount: 2)
    │   └─ ← true
    └─ ← ()

[PASS] testSafeTransferETH(uint256) (runs: 256, μ: 12749, ~: 13090)
Traces:
  [13090] SafeTransferLibTest::testSafeTransferETH(33194644780465427076338445808891870097632030972)
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::deal(0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926, 33194644780465427076338445808891870097632030972)
    │   └─ ← ()
    ├─ [0] VM::prank(0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926)
    │   └─ ← ()
    ├─ [55] SafeTransferLibTest::receive{value: 33194644780465427076338445808891870097632030972}()
    │   └─ ← ()
    └─ ← ()

[PASS] testSafeTransferFrom(address,uint256) (runs: 256, μ: 47561, ~: 48264)
Traces:
  [52578] SafeTransferLibTest::testSafeTransferFrom(0x08198855A1FcAdC0543b58D29c141A5F9e0C9292, 2265)
    ├─ [0] VM::prank(0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926)
    │   └─ ← ()
    ├─ [24523] Token::approve(SafeTransferLibTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 2265)
    │   ├─ emit Approval(owner: 0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926, spender: SafeTransferLibTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], amount: 2265)
    │   └─ ← true
    ├─ [24343] Token::transferFrom(0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926, 0x08198855A1FcAdC0543b58D29c141A5F9e0C9292, 2265)
    │   ├─ emit Transfer(from: 0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926, to: 0x08198855A1FcAdC0543b58D29c141A5F9e0C9292, amount: 2265)
    │   └─ ← true
    └─ ← ()

Test result: ok. 3 passed; 0 failed; finished in 2.77s

transmissions11 · 2022-12-14T03:21:36Z

Or something more complete:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import "solmate/utils/SafeTransferLib.sol";
import "solmate/tokens/ERC20.sol";

contract SafeTransferLibTest is Test {
    using SafeTransferLib for ERC20;

    ERC20 token;
    address user = 0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926;

    function setUp() public {
        vm.prank(user);
        token = new Token();
    }

    function testSafeTransferFrom(address to, uint256 amount) public {
        vm.prank(user);
        ERC20(token).safeApprove(address(this), amount);
        ERC20(token).safeTransferFrom(user, to, amount);
    }

    function testSafeTransfer(address to, uint256 amount) public {
        vm.prank(user);
        ERC20(token).safeTransfer(to, amount);
    }

    function testSafeTransferETH(uint256 amount) public {
        vm.assume(amount < type(uint256).max / 2);
        vm.deal(user, amount);
        vm.prank(user);
        SafeTransferLib.safeTransferETH(address(this), amount);
    }

    receive() external payable {}
    fallback() external payable {}
}

contract Token is ERC20("", "", 18) {
    constructor() {
        _mint(msg.sender, type(uint256).max);
    }
}

Running 3 tests for test/SafeTransferLib.t.sol:SafeTransferLibTest
[PASS] testSafeTransfer(address,uint256) (runs: 256, μ: 39047, ~: 39837)
Traces:
  [39837] SafeTransferLibTest::testSafeTransfer(0xf497BA995A0d145956eBb6358782F60D83556F37, 2)
    ├─ [0] VM::prank(0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926)
    │   └─ ← ()
    ├─ [29674] Token::transfer(0xf497BA995A0d145956eBb6358782F60D83556F37, 2)
    │   ├─ emit Transfer(from: 0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926, to: 0xf497BA995A0d145956eBb6358782F60D83556F37, amount: 2)
    │   └─ ← true
    └─ ← ()

[PASS] testSafeTransferETH(uint256) (runs: 256, μ: 12749, ~: 13090)
Traces:
  [13090] SafeTransferLibTest::testSafeTransferETH(33194644780465427076338445808891870097632030972)
    ├─ [0] VM::assume(true) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::deal(0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926, 33194644780465427076338445808891870097632030972)
    │   └─ ← ()
    ├─ [0] VM::prank(0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926)
    │   └─ ← ()
    ├─ [55] SafeTransferLibTest::receive{value: 33194644780465427076338445808891870097632030972}()
    │   └─ ← ()
    └─ ← ()

[PASS] testSafeTransferFrom(address,uint256) (runs: 256, μ: 47561, ~: 48264)
Traces:
  [52578] SafeTransferLibTest::testSafeTransferFrom(0x08198855A1FcAdC0543b58D29c141A5F9e0C9292, 2265)
    ├─ [0] VM::prank(0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926)
    │   └─ ← ()
    ├─ [24523] Token::approve(SafeTransferLibTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 2265)
    │   ├─ emit Approval(owner: 0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926, spender: SafeTransferLibTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], amount: 2265)
    │   └─ ← true
    ├─ [24343] Token::transferFrom(0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926, 0x08198855A1FcAdC0543b58D29c141A5F9e0C9292, 2265)
    │   ├─ emit Transfer(from: 0xA3A771A7c4AFA7f0a3f88Cc6512542241851C926, to: 0x08198855A1FcAdC0543b58D29c141A5F9e0C9292, amount: 2265)
    │   └─ ← true
    └─ ← ()

Test result: ok. 3 passed; 0 failed; finished in 2.77s

did these pass before these changes? we already have tests for this file fyi

sandybradley · 2022-12-14T13:38:17Z

My bad. The test below fails with the old version but passes with the new.

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import "forge-std/Test.sol";
import "solmate/utils/SafeTransferLib.sol";
import "solmate/tokens/ERC20.sol";

contract SafeTransferLibTest is Test {
    using SafeTransferLib for ERC20;
    using stdStorage for StdStorage;

    address FRAX = 0x853d955aCEf822Db058eb8505911ED77F175b99e;

    function writeTokenBalance(
        address who,
        address token,
        uint256 amt
    ) internal {
        stdstore.target(token).sig(ERC20(token).balanceOf.selector).with_key(who).checked_write(amt);
    }

    function testSafeTransferFrax(uint256 to, uint256 amount) public {
        vm.assume(to > 0);
        vm.assume(to != 1461501637330902918203684832716283019655932542976);
        vm.assume(amount > 0);
        vm.assume(amount < type(uint256).max / 2);
        address _to;
        assembly ("memory-safe") {
            let pos := mload(0x40)
            mstore(pos, to)
            _to := mload(pos)
        }
        writeTokenBalance(address(this), FRAX, amount);
        ERC20(FRAX).safeTransfer(_to, amount);
    }
}

Old SafeTransferLib

[376] 0x853d955aCEf822Db058eb8505911ED77F175b99e::transfer(0x0000000000000000000000000000000000000001, 1)
└─ ← "EvmError: Revert"

New SafeTransferLib

[20339] 0x853d955aCEf822Db058eb8505911ED77F175b99e::transfer(0x43c3eedE6E419Fa1B8A71781afd1B9ADd0e96cB0, 64021726494435774418933814372073409007200368138728157)
├─ emit Transfer(param0: SafeTransferLibTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], param1: 0x43c3eedE6E419Fa1B8A71781afd1B9ADd0e96cB0, param2: 64021726494435774418933814372073409007200368138728157)
└─ ← 0x0000000000000000000000000000000000000000000000000000000000000001

transmissions11 · 2023-06-13T20:11:41Z

Need to implement regressions tests for this at some point but have delayed merging this for far too long.

sambacha · 2023-06-13T20:18:26Z

Need to implement regressions tests for this at some point but have delayed merging this for far too long.

v7 release soon?

What else ya need to push it over the line for that?

Thanks for following up, let us know how we can help, cheers.

transmissions11 · 2023-06-13T20:32:06Z

v7 release soon?

tbqh ive lied about this so many times i dont even trust myself to give a timeline here anymore, but i'd like to get it wrapped up within the next month or two yea

in terms of what it takes to get over the line its mostly converting the suite away from dstest -> foundry modernities w/ forge-std + merging the backlog of PRs + auditing + incorporating optimization SoTA

sambacha · 2023-06-13T20:33:31Z

v7 release soon?

tbqh ive lied about this so many times i dont even trust myself to give a timeline here anymore, but i'd like to get it wrapped up within the next month or two yea

Ahh so obviously to be bundled with Foundry v1.0 got ya

transmissions11 · 2023-06-13T21:57:44Z

earnestly not the intent but that would be nice

🐛 Mask address args in STL

a115360

Merge branch 'main' into mask-address-args-STL

bceeec7

Update .gas-snapshot

6e48caa

transmissions11 merged commit 50e15bb into main Jun 13, 2023

sandybradley mentioned this pull request Aug 28, 2023

safeTransfer fails on flashloan callback for weird erc20 #346

Closed

c4-bot-2 mentioned this pull request Dec 3, 2023

Bug in SafeTransferLib can make SemiFungiblePositionManager not compatible with some Uniswap Pools/Tokens code-423n4/2023-11-panoptic-findings#154

Closed

This was referenced Dec 4, 2023

Some ERC20 tokens will not work with SafeTransferLib code-423n4/2023-11-panoptic-findings#253

Closed

SafeTransferLib will revert for some ERC20 tokens code-423n4/2023-11-panoptic-findings#391

Closed

c4-bot-1 mentioned this pull request Dec 11, 2023

Missing address mask for safeTransferFrom code-423n4/2023-11-panoptic-findings#460

Closed

c4-judge mentioned this pull request Dec 14, 2023

Upgraded Q -> 2 from #271 [1702573017419] code-423n4/2023-11-panoptic-findings#637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Mask address args in STL #347

🐛 Mask address args in STL #347

transmissions11 commented Dec 12, 2022

sambacha commented Dec 12, 2022

transmissions11 commented Dec 12, 2022

sandybradley commented Dec 13, 2022

sandybradley commented Dec 13, 2022

transmissions11 commented Dec 14, 2022

sandybradley commented Dec 14, 2022

transmissions11 commented Jun 13, 2023

sambacha commented Jun 13, 2023

transmissions11 commented Jun 13, 2023 •

edited

Loading

sambacha commented Jun 13, 2023

transmissions11 commented Jun 13, 2023

🐛 Mask address args in STL #347

🐛 Mask address args in STL #347

Conversation

transmissions11 commented Dec 12, 2022

Description

Checklist

sambacha commented Dec 12, 2022

transmissions11 commented Dec 12, 2022

sandybradley commented Dec 13, 2022

sandybradley commented Dec 13, 2022

transmissions11 commented Dec 14, 2022

sandybradley commented Dec 14, 2022

transmissions11 commented Jun 13, 2023

sambacha commented Jun 13, 2023

transmissions11 commented Jun 13, 2023 • edited Loading

sambacha commented Jun 13, 2023

transmissions11 commented Jun 13, 2023

transmissions11 commented Jun 13, 2023 •

edited

Loading