jasonxiale - `VestingEscrowFactory.deployVestingContract` deploys contracts using clone, which is suspicious of the reorg attack

[sherlock-admin]

jasonxiale

medium

VestingEscrowFactory.deployVestingContract deploys contracts using clone, which is suspicious of the reorg attack

Summary

Function VestingEscrowFactory.deployVestingContract deploys a new contract by calling vestingEscrowImpl.clone, and in function LibClone.clone, create is used to create new contract.

 43     function clone(address implementation) internal returns (address instance) {
 44         /// @solidity memory-safe-assembly
 45         assembly {
...
103             mstore(0x21, 0x5af43d3d93803e602a57fd5bf3)
104             mstore(0x14, implementation)
105             mstore(0x00, 0x602c3d8160093d39f33d3d3d3d363d3d37363d73)
106             instance := create(0, 0x0c, 0x35) <<<------- Here create is used 
107             // If `instance` is zero, revert.
...
116         }
117     }

Because of create is used, the contract is suspicious of the reorg attack. Suppose that:

1. Alice calls deployVestingContract twice to create two VestingEscrow, which is VE1, and VE2, then the VestingEscrowFactory.owner calls VestingEscrow.revokeAll on VE1.
2. After some time, the reorg happens, the old VE1 is now VE2, and the old VE2 is now VE1, now when VestingEscrowFactory.owner's VestingEscrow.revokeAll tx is executed, it will executed on VE1 contract which is old VE2 contract, and this is against the VestingEscrowFactory.owner's will

Vulnerability Detail

Function VestingEscrowFactory.deployVestingContract deploys a new contract by calling vestingEscrowImpl.clone, and in function LibClone.clone, create is used to create new contract.

 43     function clone(address implementation) internal returns (address instance) {
 44         /// @solidity memory-safe-assembly
 45         assembly {
...
103             mstore(0x21, 0x5af43d3d93803e602a57fd5bf3)
104             mstore(0x14, implementation)
105             mstore(0x00, 0x602c3d8160093d39f33d3d3d3d363d3d37363d73)
106             instance := create(0, 0x0c, 0x35) <<<------- Here create is used 
107             // If `instance` is zero, revert.
...
116         }
117     }

Because of create is used, the contract is suspicious of the reorg attack. Suppose that:

1. Alice calls deployVestingContract twice to create two VestingEscrow, which is VE1, and VE2, then the VestingEscrowFactory.owner calls VestingEscrow.revokeAll on VE1.
2. After some time, the reorg happens, the old VE1 is now VE2, and the old VE2 is now VE1, now when VestingEscrowFactory.owner's VestingEscrow.revokeAll tx is executed, it will executed on VE1 contract which is old VE2 contract, and this is against the VestingEscrowFactory.owner's will

Impact

reorg attack

Code Snippet

https://github.com/sherlock-audit/2024-01-rio-vesting-escrow/blob/main/rio-vesting-escrow/src/VestingEscrowFactory.sol#L64-L69

Tool used

Manual Review

Recommendation

using LibClone.cloneDeterministic instead of LibClone.clone

Duplicate of #66

[sherlock-admin2]

2 comment(s) were left on this issue during the judging contest.

_rahul commented:

Invalid: Chain re-org and network liveness related issues are not considered valid.

pratraut commented:

'invalid as reorg will re-execute those txs and owner will have appropriate state'