Anyone can deposit and payback in name of other user

[rotcivegaf]

Git branch: H02

Affected smart contract

https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/abstracts/BaseRouter.sol#L294-L316

https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/abstracts/BaseRouter.sol#L135-L146

https://github.com/Fujicracy/fuji-v2/blob/1b939ec84af137db430fc2aa1b4c6f15e5254003/packages/protocol/src/abstracts/BaseRouter.sol#L165-L176

Description

The _bundleInternal function of the BaseRouter contract has 2 paths where it uses the _safePullTokenFrom function, which has a security flaw, as anyone can use it on behalf of another user. The paths are those of deposit and payback. This line is the problematic one: if (sender != address(this) && (sender == owner || sender == msg.sender)) {

Attack scenario

Deposit scenario:

1. Alice has a 1 ETHER of X token
2. Alice approves the router to manage 1 ETHER of his tokens
3. In this instant anyone can deposit this 1 ETHER of alice using the router and they do so
4. Alice ends up with her deposited ether without her allowing it

Payback scenario:

5. Continue the before scenario, Alice borrow some amount of tokens
6. In this instant anyone can payback the debt of alice using his tokens and they do so
7. Alice ends up with her payback his debt without her allowing it

Recommendation

Consider change the check of _safePullTokenFrom function:

@@ -310,8 +310,10 @@ abstract contract BaseRouter is SystemAccessControl, IRouter {
   )
     internal
   {
-    if (sender != address(this) && (sender == owner || sender == msg.sender)) {
-      ERC20(token).safeTransferFrom(sender, address(this), amount);
+    if (sender != address(this) && sender == owner) {
+      if (sender == msg.sender) {
+        ERC20(token).safeTransferFrom(sender, address(this), amount);
+      }
     }
   }

Another option is to implement a approve system such as the EIP20

[0xdcota]

The _safePullTokenFrom is updated and will be merged in PR #425.