mirror of
https://github.com/serai-dex/serai.git
synced 2025-04-16 11:11:56 +00:00
Fix the ability for a malicious adversary to snipe ERC20s out via re-entrancy from the ERC20 contract
This commit is contained in:
parent
17cc10b3f7
commit
0484113254
3 changed files with 43 additions and 3 deletions
processor/ethereum/router
|
@ -63,6 +63,8 @@ interface IRouterWithoutCollisions {
|
|||
/// @notice The call to an ERC20's `transferFrom` failed
|
||||
error TransferFromFailed();
|
||||
|
||||
/// @notice The code wasn't to-be-executed by self
|
||||
error CodeNotBySelf();
|
||||
/// @notice A non-reentrant function was re-entered
|
||||
error Reentered();
|
||||
|
||||
|
|
|
@ -406,9 +406,8 @@ contract Router is IRouterWithoutCollisions {
|
|||
arbitrarily called). We accordingly don't need to be worried about return bombs here.
|
||||
*/
|
||||
// slither-disable-next-line return-bomb
|
||||
(bool erc20Success, bytes memory res) = address(coin).call{ gas: ERC20_GAS }(
|
||||
abi.encodeWithSelector(selector, to, amount)
|
||||
);
|
||||
(bool erc20Success, bytes memory res) =
|
||||
address(coin).call{ gas: ERC20_GAS }(abi.encodeWithSelector(selector, to, amount));
|
||||
|
||||
/*
|
||||
Require there was nothing returned, which is done by some non-standard tokens, or that the
|
||||
|
@ -504,6 +503,25 @@ contract Router is IRouterWithoutCollisions {
|
|||
*/
|
||||
/// @param code The code to execute
|
||||
function executeArbitraryCode(bytes memory code) external payable {
|
||||
/*
|
||||
execute assumes that from the time it reads `_smartContractNonce` until the time it calls this
|
||||
function, no mutations to it will occur. If any mutations could occur, it'd lead to a fault
|
||||
where tokens could be sniped by:
|
||||
|
||||
1) An out occurring, transferring tokens to an about-to-be-deployed smart contract
|
||||
2) The token contract re-entering the Router to deploy a new smart contract which claims the
|
||||
tokens
|
||||
3) The Router then deploying the intended smart contract (ignoring whatever result it may
|
||||
have)
|
||||
|
||||
This does assume a malicious token, or a token with callbacks which can be set by a malicious
|
||||
adversary, yet the way to ensure it's a non-issue is to not allow other entities to mutate
|
||||
`_smartContractNonce`.
|
||||
*/
|
||||
if (msg.sender != address(this)) {
|
||||
revert CodeNotBySelf();
|
||||
}
|
||||
|
||||
// Because we're creating a contract, increment our nonce
|
||||
_smartContractNonce += 1;
|
||||
|
||||
|
|
|
@ -706,6 +706,26 @@ async fn test_erc20_top_level_transfer_in_instruction() {
|
|||
test.publish_in_instruction_tx(tx, coin, amount, &shorthand).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_execute_arbitrary_code() {
|
||||
let test = Test::new().await;
|
||||
|
||||
assert!(matches!(
|
||||
test
|
||||
.call_and_decode_err(TxLegacy {
|
||||
chain_id: None,
|
||||
nonce: 0,
|
||||
gas_price: 100_000_000_000,
|
||||
gas_limit: 1_000_000,
|
||||
to: test.router.address().into(),
|
||||
value: U256::ZERO,
|
||||
input: crate::abi::executeArbitraryCodeCall::new((vec![].into(),)).abi_encode().into(),
|
||||
})
|
||||
.await,
|
||||
IRouterErrors::CodeNotBySelf(IRouter::CodeNotBySelf {})
|
||||
));
|
||||
}
|
||||
|
||||
// Code which returns true
|
||||
#[rustfmt::skip]
|
||||
fn return_true_code() -> Vec<u8> {
|
||||
|
|
Loading…
Reference in a new issue