MorpheusAI

Summary

sendDepositToken and sendMintMessage functions cannot be called by "distribution" address due to wrong onlyDistribution modifier implementation

Vulnerability Details

Here's the require statement in the "onlyDistribution" modifier:

require(_msgSender() == distribution, "L1S: invalid sender");

The issue with the require statement is the use of "_msgSender()". "_msgSender()" is a function in OwnableUpgradeable contract imported by L1Sender.sol contract. The functioin returns the owner of the contract.

Invariably, "_msgSender()" represents the owner of the contract.

Now back to the require statement. What the require statement means is that "owner must be equal to distribution". And this is not the intention behind the modifier. The intention is "msg.sender must be equal to distribution". This is because "distribution" address is specifically set in "L1Sender__init" function. The function also calls "__Ownable_init()".

Here's a simple, similar contract test that shows this bug:

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

import {Test, console2} from "forge-std/Test.sol";

contract Sender {
address public owner;
address public distributor;

constructor(address _distributor) {
owner = msg.sender;
distributor = _distributor;
}

modifier onlyDistribution() {
require(_msgSender() == distributor, "invalid sender");
_;
}

function _msgSender() public view returns (address) {
return owner;
}

function add() public view onlyDistribution returns (uint256) {
return 4 + 4;

}
}

In the below test, the "owner" which is the contract in this case (auto set as owner) calls the "add" function and it passes:

contract SenderTest is Test {
Sender sender;
// address distributor = vm.addr(1);

function setUp() public {
    sender = new Sender(address(this));
}

function testAdd() public {
    vm.prank(address(this));
    sender.add();
    console2.log(sender.add());
    vm.stopPrank();
}

}

Here's the traces:

forge test -vvv
[⠒] Compiling...
[⠢] Compiling 2 files with 0.8.22
[⠆] Solc 0.8.22 finished in 1.48s
Compiler run successful!

Running 1 test for test/Re.t.sol:SenderTest
[PASS] testAdd() (gas: 16753)
Logs:
8

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.03ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Here's the test when "distribution" calls "add" function. It reverts:

contract SenderTest is Test {
Sender sender;
address distribution = vm.addr(1);

function setUp() public {
    sender = new Sender(distribution); //sets distribution here
}

function testAdd() public {
    vm.prank(distribution); //sets distribution as the caller
    sender.add();
    console2.log(sender.add());
    vm.stopPrank();
}

}

Here's the traces:

forge test -vvv
[⠒] Compiling...
[⠢] Compiling 1 files with 0.8.22
[⠆] Solc 0.8.22 finished in 1.48s
Compiler run successful!

Running 1 test for test/Reentrancy.t.sol:SenderTest
[FAIL. Reason: revert: invalid sender] testAdd() (gas: 14601)
Traces:
[14601] SenderTest::testAdd()
├─ [0] VM::prank(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)
│ └─ ← ()
├─ [4543] Sender::add() [staticcall]
│ └─ ← revert: invalid sender
└─ ← revert: invalid sender

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.29ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/Re.t.sol:SenderTest
[FAIL. Reason: revert: invalid sender] testAdd() (gas: 14601)

Encountered a total of 1 failing tests, 0 tests succeeded

Impact

"distribution" address will revert if it calls "sendDepositToken" and "sendMintMessage" functions. Only the owner can call those functions without reverting which is not intended.

Tools Used

Manual review

Recommendations

Use msg.sender instead:
require(msg.sender == distribution, "L1S: invalid sender");