Skip to content

Commit

Permalink
Audit fix additions (#64)
Browse files Browse the repository at this point in the history
* Force ConduitMover to withdraw only lot

* Add clarification on Buffer-Conduits movement rate limiting responsibilities

* Minor changes
  • Loading branch information
oldchili authored Oct 10, 2023
1 parent 4928ce0 commit 5dba577
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 3 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ An interface which each Conduit should implement.
- AllocatorDAOs can not incur a loss of more than the debt ceiling (`line`) of their respective `ilk`.
- A funnel operator (whether a facilitator or an automated contract) can not incur a loss of more than `cap` amount of funds per `era` interval for a specific configuration. This includes not being able to move funds directly to any unknown address that the AllocatorDAO Proxy did not approve.
- A keeper's maximum loss must be bounded by `cap` amount of funds per `era` (as for a funnel operator) but is additionally constrainted by `lot` (or `amt0` and `amt1`) amount of funds per `hop` for a specific configuration. Moreover, a keeper's execution must guarantee a minimum amount of output tokens, defined by `req` (or `req0` and `req1`) for a specific configuration.
- If a rate limit is needed for depositing or withdrawing in a specific Conduit (in order to limit the harm a rogue facilitator can cause), it is the responsibility of the Conduit itself to implement it.

## Technical Assumptions:
- A `uint32` is suitable for storing timestamps or time intervals in the funnels, as the current version of the Allocation System is expected to be deprecated long before 2106.
Expand Down
10 changes: 7 additions & 3 deletions src/funnels/automation/ConduitMover.sol
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ pragma solidity ^0.8.16;

interface ConduitLike {
function deposit(bytes32, address, uint256) external;
function withdraw(bytes32, address, uint256) external;
function withdraw(bytes32, address, uint256) external returns (uint256);
}

contract ConduitMover {
Expand Down Expand Up @@ -99,8 +99,12 @@ contract ConduitMover {
unchecked { configs[from][to][gem].num = cfg.num - 1; }
configs[from][to][gem].zzz = uint32(block.timestamp);

if (from != buffer) ConduitLike(from).withdraw(ilk, gem, cfg.lot);
if (to != buffer) ConduitLike(to).deposit(ilk, gem, cfg.lot);
if (from != buffer) {
require(ConduitLike(from).withdraw(ilk, gem, cfg.lot) == cfg.lot, "ConduitMover/lot-withdraw-failed");
}
if (to != buffer) {
ConduitLike(to).deposit(ilk, gem, cfg.lot);
}

emit Move(from, to, gem, cfg.lot);
}
Expand Down
8 changes: 8 additions & 0 deletions test/funnels/automation/ConduitMover.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,12 @@ contract ConduitMoverTest is DssTest {
vm.expectRevert("ConduitMover/exceeds-num");
vm.prank(KEEPER); mover.move(conduit1, conduit2, address(0x123));
}

function testMoveLotWithdrawFail() public {
vm.prank(FACILITATOR); mover.setConfig(conduit1, buffer, USDC, 10, 1 hours, uint128(3_100 * 10**6));
assertEq(GemLike(USDC).balanceOf(conduit1), 3_000 * 10**6);

vm.expectRevert("ConduitMover/lot-withdraw-failed");
vm.prank(KEEPER); mover.move(conduit1, buffer, USDC);
}
}

0 comments on commit 5dba577

Please sign in to comment.